Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

misc/wasm: wasm_exec.js in Go 1.15 cannot be used alongside another JS library that overrides global.require #40730

Closed
tliron opened this issue Aug 12, 2020 · 16 comments
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@tliron
Copy link

tliron commented Aug 12, 2020

What version of Go are you using (go version)?

$ go version
1.15

Does this issue reproduce with the latest release?

It only happens in the latest release.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOHOSTARCH="amd64"
GOHOSTOS="linux"

(Fedora 32)

What did you do?

Running a simple Go-compiled WASM file using wasm_exec.js inside the Firefox browser (ver 79.0).

What did you expect to see?

I expect it to work, as in the Go 1.14 version. However some changes in wasm_exec.js make it break, at least in Firefox.

What did you see instead?

Uncaught TypeError: can't convert undefined to object
    <anonymous> http://localhost:8000/js/wasm_exec.js:31
    <anonymous> http://localhost:8000/js/wasm_exec.js:588
wasm_exec.js:31:14

Link to source code

But that's not the only issue. If I comment out the additions in line 31 I get this:

Uncaught ReferenceError: module is not defined
    <anonymous> http://localhost:8000/js/wasm_exec.js:560
    <anonymous> http://localhost:8000/js/wasm_exec.js:588

Commenting out line 560 will finally make it work.

Link to source code

See also issue #40446 which is what seems to have added line 31.

@dmitshur dmitshur changed the title wasm_exec.js is broken for browsers in Go 1.15 wasm: wasm_exec.js is broken for browsers in Go 1.15 Aug 12, 2020
@dmitshur dmitshur assigned dmitshur and unassigned dmitshur Aug 12, 2020
@dmitshur dmitshur added arch-wasm WebAssembly issues NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 12, 2020
@dmitshur dmitshur added this to the Backlog milestone Aug 12, 2020
@dmitshur dmitshur changed the title wasm: wasm_exec.js is broken for browsers in Go 1.15 misc/wasm: wasm_exec.js is broken for browsers in Go 1.15 Aug 12, 2020
@dmitshur
Copy link
Contributor

I've not observed these errors in a recent stable version of Chrome (84.0.4147.105). I tried with Firefox 78.0a1 just now, and didn't see these errors either. I tested on macOS.

/cc @neelance @nao20010128nao

@tliron
Copy link
Author

tliron commented Aug 12, 2020

I just tried with Chrome 84.0.4147.125, too, and got the exact same errors as with Firefox.

@Lesmiscore
Copy link
Contributor

Lesmiscore commented Aug 13, 2020

Hi, I'm who requested to add line 31.
I found out that line 31 is completely strange(see below), but browsers shouldn't have fs module or even require().
This should be checked at outer if.
Is there any libraries that exposes require() loaded?

And line 31 must be replaced with:

if (typeof fs === "object" && Object.keys(fs).length !== 0) {

@tliron
Copy link
Author

tliron commented Aug 13, 2020

I would similarly recommend changing line 560 to something like this:

((typeof module !== "undefined") && (global.require.main === module)) &&

@neelance
Copy link
Member

I just love how workaround leads to workaround leads to workaround in the JavaScript world... But yes, I guess we can add those changes to make it more robust against weird environments.

@tliron
Copy link
Author

tliron commented Aug 13, 2020

@neelance The "weird" environment you are mentioning here is a standard web browser. These adaptations are meant to detect the specialized environment of Node.js and enable special hooks. Anyway, I hope that in the future changes to this file will be accompanied with actual testing in the environments which it is supposed to support.

@neelance
Copy link
Member

@tliron No it is not. A standard web browser has no require so you can not even end up in line 31 which is crashing for you (see if (!global.fs && global.require) {) and it does not complain about missing module as long as strict mode is not enabled. The issues that you are seeing are due to some bundler that you are using. The reference https://github.com/golang/go/blob/master/misc/wasm/wasm_exec.html works perfectly fine.

@tliron
Copy link
Author

tliron commented Aug 13, 2020

@neelance I am not using any bundler. You can see my exact usage here (see the HTML source for the page). I did some more digging and I see that the ACE library I am using is defining global.require as a stub (probably to solve its own challenges in terms of working in different environments).

What is wrong with the suggested fix by @nao20010128nao ? Not only are we checking that there is a require, but we're also checking that it does what we expect it to do in terms of importing a module.

As for line 560: I am not enabling strict mode. The exception is caused because global.require was defined above in line 26, and so the boolean statement in line 559 will evaluate to true, and then because of the && it will continue to evaluate line 560. So, again, my suggested fix here is to make this clause more robust in this situation, a situation in which there is a require but there is no module. What is wrong with that?

I'm personally OK patching wasm_exec.js myself, but I'm trying to make the Go distribution better by reporting this issue. ACE is a very popular library. And, I wouldn't be surprised if other JS libraries define require, too. And what is wrong with people using bundlers? Those are also popular. As it stands right now the wasm_exec.js included in Go 1.15 will not work with ACE, which could be frustrating to potential users, especially those upgrading from Go 1.14, in which it did work.

@dmitshur
Copy link
Contributor

dmitshur commented Aug 13, 2020

@tliron Thank you for investigating further and providing additional information. Investigating and fixing an issue in Go is not possible when we can't understand and reproduce it, and your latest comment helps with that a lot.

@dmitshur dmitshur changed the title misc/wasm: wasm_exec.js is broken for browsers in Go 1.15 misc/wasm: wasm_exec.js in Go 1.15 cannot be used alongside another JS library that overrides global.require Aug 13, 2020
@neelance
Copy link
Member

What is wrong with the suggested fix by @nao20010128nao ?

What is wrong with that?

@tliron Nothing is wrong. I wrote above:

But yes, I guess we can add those changes to make it more robust against weird environments.

I was only saying that this is a very typical situation for the JavaScript world:

I see that the ACE library I am using is defining global.require as a stub (probably to solve its own challenges in terms of working in different environments).

This is a workaround ("solve its own challenges in terms of working in different environments") leading to a workaround (add logic to detect this to wasm_exec.js).

@tliron
Copy link
Author

tliron commented Aug 13, 2020

@neelance Thanks! I was just confused with your use of the word "weird". It seemed like you were implying that this was my fault for misusing the library.

Anyway, we might not like it, but these kinds of hacky workarounds are the nature of working with web browsers. In the end this library just provides the glue so that we can use Go instead...

@gopherbot
Copy link

Change https://golang.org/cl/248758 mentions this issue: misc/wasm: make wasm_exec more robust against uncommon environments

@neelance
Copy link
Member

I have prepared https://golang.org/cl/248758. @tliron Does this change look good?

@tliron
Copy link
Author

tliron commented Aug 15, 2020

@neelance Thanks for asking, to me it looks fine! I also just tested it and it works in my environment.

@tonyhb
Copy link

tonyhb commented Aug 15, 2020

Confirmed it works in my environment too, go 1.15, Chrome/Firefox, but breaks without these changes.

@Lesmiscore
Copy link
Contributor

@neelance It looks good, and it worked in my test environment.

@golang golang locked and limited conversation to collaborators Aug 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants