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

Comments

@tliron
Copy link

@tliron 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 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
Member

@dmitshur dmitshur commented Aug 12, 2020

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 tliron commented Aug 12, 2020

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

@nao20010128nao
Copy link
Contributor

@nao20010128nao nao20010128nao 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 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

@neelance neelance commented Aug 13, 2020

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 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

@neelance neelance commented Aug 13, 2020

@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 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
Member

@dmitshur 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

@neelance neelance commented Aug 13, 2020

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 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

@gopherbot gopherbot commented Aug 15, 2020

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

@neelance
Copy link
Member

@neelance neelance commented Aug 15, 2020

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

@tliron
Copy link
Author

@tliron 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 tonyhb commented Aug 15, 2020

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

@nao20010128nao
Copy link
Contributor

@nao20010128nao nao20010128nao commented Aug 16, 2020

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

@gopherbot gopherbot closed this in 758ac37 Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.