-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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: add polyfill for TextEncoder/TextDecoder for Edge support #27296
misc/wasm: add polyfill for TextEncoder/TextDecoder for Edge support #27296
Conversation
Message from Gerrit User 5976: Patch Set 1: Congratulations on opening your first change. Thank you for your contribution! Next steps: Most changes in the Go project go through a few rounds of revision. This can be During May-July and Nov-Jan the Go project is in a code freeze, during which Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 13620: Patch Set 1: Polyfills should be provided by the user. Please move this to the other polyfill in wasm_exec.html. Who is the author of this polyfill? Are we allowed to use it? Do we need to add attribution? Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 28917: Patch Set 1:
The author is the GitHub user Yaffle, and it was taken from their gists here: Since it doesn't seem to have a license attached, I'll assume we cannot use it. I tested the following polyfill and that seems to work fine as well, it's also licensed under "Apache License 2.0" which seems like it should be OK? (Though I'm not sure when it comes to licensing) I also had a look for other implementations on GitHub and looked at their licensing, but it seemed to either use "Apache License 2.0", "Creative Commons Attribution 4.0 International License" or be unclear.
If this is a problem, is it OK if I rewrite the implementation so that it doesn't resemble these? Thoughts? Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 24096: Patch Set 1:
I have commented on Yaffle's gist to add a license. Apache License 2.0 is fine too, as long as we keep the license notice. I think we have an open issue somewhere to aggregate all the licences used in the code in a single big license file. Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 12446: Uploaded patch set 3: New patch set was added with same tree, parent, and commit message as Patch Set 2. Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 28917: Patch Set 3:
Alright thanks! Assuming Yaffle adds a license or gives the OK, is there any other changes you'd like me to make? Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 13620: Patch Set 3: (4 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 12446: Uploaded patch set 5: New patch set was added with same tree, parent, and commit message as Patch Set 4. Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
6508d1a
to
da7f55a
Compare
Message from Gerrit User 28917: Patch Set 3:
I've made the changes requested. Perhaps the encoder/decoder const variables in wasm_exec.js should be instantiated in the Go class constructor or similar? (So the polyfill can be included after the script but before initialization of Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
…ace polyfill before wasm_exec.js to fix crashing in Edge Fixes golang#27295
Message from Gerrit User 12446: Uploaded patch set 7: New patch set was added with same tree, parent, and commit message as Patch Set 6. Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
da7f55a
to
75c2b51
Compare
Message from Gerrit User 12446: Uploaded patch set 9: New patch set was added with same tree, parent, and commit message as Patch Set 8. Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 12446: Uploaded patch set 11: New patch set was added with same tree, parent, and commit message as Patch Set 10. Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 28917: Patch Set 11: (4 comments)
Commented too soon and missed a semicolon on the "window.TextDecoder = function(){};". Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 24096: Patch Set 11: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 28917: Patch Set 11: (1 comment)
Added comment to your feedback item Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 24096: Patch Set 11: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 24096: Patch Set 11: Sorry for commenting this late in the review. But I just saw a polyfill implementation of textencoder in the MDN site https://developer.mozilla.org/en-US/docs/Web/API/TextEncoder#Polyfill. I think that is a better candidate to avoid licensing issues. Also, the MDN docs point to this repo - https://github.com/inexorabletash/text-encoding which has a public license and has both encoding and decoding functions. I know its a pain to change the algo at this stage of the review and I have no problems as such with the existing implementation. Up to Richard/Brad to take a final call. Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 28917: Patch Set 11:
I'm happy to do this, I just want confirmation first! Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 13620: Patch Set 11:
Unfortunately the gist https://gist.github.com/Yaffle/5458286 still has no license at all. The license of MDN code snippets is CC0, which is good, but there seems to be no polyfill for the decoder. The project https://github.com/inexorabletash/text-encoding is in the public domain as well, but it is quite large. What about just referencing it: <script src="https://unpkg.com/[email protected]/lib/encoding.js"></script>Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 24096: Patch Set 11:
That works. But I wouldn't want to inject a large script unconditionally. Let's check for TextEncoder support and then inject it if it's not there. Something like this - Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 13620: Patch Set 11:
You could use https://cdn.jsdelivr.net/npm/[email protected]/lib/encoding.min.js, which is only 5.6 KB on the wire. At this size I would prefer the simpler code. Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 24096: Patch Set 11:
SGTM. Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 28917: Patch Set 11:
I'm happy with either JS files being loaded. Keep in mind that make the polyfill work, I'd have to restructure the code to something like this: <!doctype html> <title>Go wasm</title> <script> let go; let mod, inst;
Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 24096: Patch Set 11: Richard meant to include https://cdn.jsdelivr.net/npm/[email protected]/lib/encoding.min.js directly in a script tag. Then you don't need any additional complexity. Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
This PR (HEAD: 07a8bfe) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/131718 to see it. Tip: You can toggle comments from me using the |
This PR (HEAD: 00722c4) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/131718 to see it. Tip: You can toggle comments from me using the |
Message from Gerrit User 28917: Patch Set 11:
Alright sounds good to me! Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 13620: Patch Set 13: Code-Review+1 (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
This PR (HEAD: a587eda) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/131718 to see it. Tip: You can toggle comments from me using the |
Message from Gerrit User 24096: Patch Set 14: Code-Review+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 28917: Patch Set 14: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
Message from Gerrit User 13620: Patch Set 14: Code-Review+2 Please don’t reply on this GitHub thread. Visit golang.org/cl/131718. |
This PR is being closed because golang.org/cl/131718 has been merged. |
Edge supports WASM but not TextEncoder or TextDecoder. This PR adds a polyfill to `misc/wasm/wasm_exec.js` to fix this. Fixes #27295 Change-Id: Ie35ee5604529b170a5dc380eb286f71bdd691d3e GitHub-Last-Rev: a587eda GitHub-Pull-Request: #27296 Reviewed-on: https://go-review.googlesource.com/131718 Reviewed-by: Agniva De Sarker <[email protected]> Reviewed-by: Richard Musiol <[email protected]>
Edge supports WASM but not TextEncoder or TextDecoder.
This PR adds a polyfill to
misc/wasm/wasm_exec.js
to fix this.Fixes #27295