[go] misc/wasm: add polyfill for TextEncoder/TextDecoder for Edge support
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded this change for review.
misc/wasm: add polyfill for TextEncoder/TextDecoder for Edge support
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: 08f86548d99d3ae10ae6c08645059c2cfcc3a798
GitHub-Pull-Request: golang/go#27296
---
M misc/wasm/wasm_exec.js
1 file changed, 76 insertions(+), 0 deletions(-)
diff --git a/misc/wasm/wasm_exec.js b/misc/wasm/wasm_exec.js
index 94b9552..6fde8036 100644
--- a/misc/wasm/wasm_exec.js
+++ b/misc/wasm/wasm_exec.js
@@ -27,6 +27,82 @@
global.TextEncoder = util.TextEncoder;
global.TextDecoder = util.TextDecoder;
} else {
+ // Add polyfill for TextEncoder and TextDecoder for Microsoft Edge 17/18 support
+ // https://caniuse.com/#feat=textencoder
+ if (!window.TextEncoder) {
+ TextEncoder = function(){}
+ TextEncoder.prototype.encode = function (string) {
+ var octets = [];
+ var length = string.length;
+ var i = 0;
+ while (i < length) {
+ var codePoint = string.codePointAt(i);
+ var c = 0;
+ var bits = 0;
+ if (codePoint <= 0x0000007F) {
+ c = 0;
+ bits = 0x00;
+ } else if (codePoint <= 0x000007FF) {
+ c = 6;
+ bits = 0xC0;
+ } else if (codePoint <= 0x0000FFFF) {
+ c = 12;
+ bits = 0xE0;
+ } else if (codePoint <= 0x001FFFFF) {
+ c = 18;
+ bits = 0xF0;
+ }
+ octets.push(bits | (codePoint >> c));
+ c -= 6;
+ while (c >= 0) {
+ octets.push(0x80 | ((codePoint >> c) & 0x3F));
+ c -= 6;
+ }
+ i += codePoint >= 0x10000 ? 2 : 1;
+ }
+ return octets;
+ };
+ }
+ if (!window.TextDecoder) {
+ TextDecoder = function(){}
+ TextDecoder.prototype.decode = function (octets) {
+ var string = "";
+ var i = 0;
+ while (i < octets.length) {
+ var octet = octets[i];
+ var bytesNeeded = 0;
+ var codePoint = 0;
+ if (octet <= 0x7F) {
+ bytesNeeded = 0;
+ codePoint = octet & 0xFF;
+ } else if (octet <= 0xDF) {
+ bytesNeeded = 1;
+ codePoint = octet & 0x1F;
+ } else if (octet <= 0xEF) {
+ bytesNeeded = 2;
+ codePoint = octet & 0x0F;
+ } else if (octet <= 0xF4) {
+ bytesNeeded = 3;
+ codePoint = octet & 0x07;
+ }
+ if (octets.length - i - bytesNeeded > 0) {
+ var k = 0;
+ while (k < bytesNeeded) {
+ octet = octets[i + k + 1];
+ codePoint = (codePoint << 6) | (octet & 0x3F);
+ k += 1;
+ }
+ } else {
+ codePoint = 0xFFFD;
+ bytesNeeded = octets.length - i;
+ }
+ string += String.fromCodePoint(codePoint);
+ i += bytesNeeded + 1;
+ }
+ return string
+ };
+ }
+
if (typeof window !== "undefined") {
window.global = window;
} else if (typeof self !== "undefined") {
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.
Richard Musiol (Gerrit)
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?
Jake Bentvelzen (Gerrit)
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?
The author is the GitHub user Yaffle, and it was taken from their gists here:
https://gist.github.com/Yaffle/5458286
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)
https://github.com/samthor/fast-text-encoding/
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. ie.
If this is a problem, is it OK if I rewrite the implementation so that it doesn't resemble these?
Alternatively, I could just change this PR to simply be a code comment that tells the user that if they want Edge support, they will need a polyfill for TextEncoder/TextDecoder.
Thoughts?
Agniva De Sarker (Gerrit)
Patch Set 1:
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?
The author is the GitHub user Yaffle, and it was taken from their gists here:
https://gist.github.com/Yaffle/5458286Since 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)
https://github.com/samthor/fast-text-encoding/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. ie.
If this is a problem, is it OK if I rewrite the implementation so that it doesn't resemble these?
Alternatively, I could just change this PR to simply be a code comment that tells the user that if they want Edge support, they will need a polyfill for TextEncoder/TextDecoder.Thoughts?
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.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #2 to this change.
misc/wasm: add polyfill for TextEncoder/TextDecoder for Edge support
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: 144a054c28be25b40c180d2fc8cf3aa47fb75217
GitHub-Pull-Request: golang/go#27296
---
M misc/wasm/wasm_exec.html
1 file changed, 76 insertions(+), 0 deletions(-)
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #3 to this change.
misc/wasm: add polyfill for TextEncoder/TextDecoder for Edge support
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: 144a054c28be25b40c180d2fc8cf3aa47fb75217
GitHub-Pull-Request: golang/go#27296
---
M misc/wasm/wasm_exec.html
1 file changed, 76 insertions(+), 0 deletions(-)
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Jake Bentvelzen (Gerrit)
Patch Set 1:
Patch Set 1:
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?
The author is the GitHub user Yaffle, and it was taken from their gists here:
https://gist.github.com/Yaffle/5458286Since 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)
https://github.com/samthor/fast-text-encoding/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. ie.
If this is a problem, is it OK if I rewrite the implementation so that it doesn't resemble these?
Alternatively, I could just change this PR to simply be a code comment that tells the user that if they want Edge support, they will need a polyfill for TextEncoder/TextDecoder.Thoughts?
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.
Alright thanks!
I've updated the PR so that the polyfill is in wasm_exec.html.
Assuming Yaffle adds a license or gives the OK, is there any other changes you'd like me to make?
Do I need to squash my commits / rebase as well?
Richard Musiol (Gerrit)
4 comments:
File misc/wasm/wasm_exec.html:
Patch Set #3, Line 27:
TextEncoder = function(){}I would prefer window.TextEncoder here, because otherwise it looks a bit like a local variable.
Also: Missing semicolon.
Patch Set #3, Line 29:
var octets = [];Please use "const" and "let", to be consistent with the rest of the file.
Patch Set #3, Line 61:
TextDecoder = function(){}Same here.
Patch Set #3, Line 96:
return stringMissing semicolon.
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #4 to this change.
misc/wasm: add polyfill for TextEncoder/TextDecoder for Edge support
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: 6508d1a0c76d11fbab4cac3259701a4b3dcf3c96
GitHub-Pull-Request: golang/go#27296
---
M misc/wasm/wasm_exec.html
1 file changed, 77 insertions(+), 0 deletions(-)
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #5 to this change.
misc/wasm: add polyfill for TextEncoder/TextDecoder for Edge support
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: 6508d1a0c76d11fbab4cac3259701a4b3dcf3c96
GitHub-Pull-Request: golang/go#27296
---
M misc/wasm/wasm_exec.html
1 file changed, 77 insertions(+), 0 deletions(-)
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Jake Bentvelzen (Gerrit)
Patch Set 3:
(4 comments)
I've made the changes requested.
I've also moved the polyfill to execute before "wasm_exec.js" because `TextEncoder` and `TextDecoder` are referenced immediately when the script is included, causing a crash in Edge.
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 `new Go()`)
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #6 to this change.
misc/wasm: add polyfill for TextEncoder/TextDecoder for Edge support
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: da7f55aa1ff452579c41ed00052d3ce9917f09eb
GitHub-Pull-Request: golang/go#27296
---
M misc/wasm/wasm_exec.html
1 file changed, 77 insertions(+), 0 deletions(-)
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #7 to this change.
misc/wasm: add polyfill for TextEncoder/TextDecoder for Edge support
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: da7f55aa1ff452579c41ed00052d3ce9917f09eb
GitHub-Pull-Request: golang/go#27296
---
M misc/wasm/wasm_exec.html
1 file changed, 77 insertions(+), 0 deletions(-)
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #8 to this change.
misc/wasm: add polyfill for TextEncoder/TextDecoder for Edge support
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: 75c2b517e68a92133b53ae8c827f525566d6e34c
GitHub-Pull-Request: golang/go#27296
---
M misc/wasm/wasm_exec.html
1 file changed, 77 insertions(+), 0 deletions(-)
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #9 to this change.
misc/wasm: add polyfill for TextEncoder/TextDecoder for Edge support
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: 75c2b517e68a92133b53ae8c827f525566d6e34c
GitHub-Pull-Request: golang/go#27296
---
M misc/wasm/wasm_exec.html
1 file changed, 77 insertions(+), 0 deletions(-)
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #10 to this change.
misc/wasm: add polyfill for TextEncoder/TextDecoder for Edge support
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: 381b5413619434b819df753f01cefd2222a91af8
GitHub-Pull-Request: golang/go#27296
---
M misc/wasm/wasm_exec.html
1 file changed, 77 insertions(+), 0 deletions(-)
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #11 to this change.
misc/wasm: add polyfill for TextEncoder/TextDecoder for Edge support
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: 381b5413619434b819df753f01cefd2222a91af8
GitHub-Pull-Request: golang/go#27296
---
M misc/wasm/wasm_exec.html
1 file changed, 77 insertions(+), 0 deletions(-)
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Jake Bentvelzen (Gerrit)
Patch Set 3:
Patch Set 3:
(4 comments)
I've made the changes requested.
I've also moved the polyfill to execute before "wasm_exec.js" because `TextEncoder` and `TextDecoder` are referenced immediately when the script is included, causing a crash in Edge.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 `new Go()`)
Commented too soon and missed a semicolon on the "window.TextDecoder = function(){};".
Rebased on master and also fixed that!
4 comments:
File misc/wasm/wasm_exec.html:
Patch Set #3, Line 27:
let bits = 0;I would prefer window.TextEncoder here, because otherwise it looks a bit like a local variable. […]
Done
Patch Set #3, Line 29:
c = 0;
Please use "const" and "let", to be consistent with the rest of the file.
Done
Patch Set #3, Line 61:
if (octet <= 0x7F) {Same here.
Done
Patch Set #3, Line 96:
const source = await (await resp).arrayBuffer();Missing semicolon.
Done
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Agniva De Sarker (Gerrit)
1 comment:
File misc/wasm/wasm_exec.html:
Patch Set #11, Line 20:
TextEncoder.prototype.encode = function (string) {Let us use the arrow syntax `() => {}` everywhere to be consistent.
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Jake Bentvelzen (Gerrit)
Patch Set 11:
(1 comment)
Added comment to your feedback item
1 comment:
Patch Set #11, Line 20:
TextEncoder.prototype.encode = function (string) {Let us use the arrow syntax `() => {}` everywhere to be consistent.
We are not using `this` in the prototype functions so I'm not convinced using fat arrow syntax here is a good idea.
As per recommendations found in the link below, I think we should leave the prototype functions alone:
https://stackoverflow.com/a/23045200
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Agniva De Sarker (Gerrit)
1 comment:
Patch Set #11, Line 20:
TextEncoder.prototype.encode = function (string) {
We are not using `this` in the prototype functions so I'm not convinced using fat arrow syntax here […]
That's true actually. Makes sense to me.
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Agniva De Sarker (Gerrit)
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.
Jake Bentvelzen (Gerrit)
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.
I'm happy to do this, I just want confirmation first!
Richard Musiol (Gerrit)
Patch Set 11:
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.
I'm happy to do this, I just want confirmation first!
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:
<!-- Polyfill for TextEncoder and TextDecoder -->
<script src="https://unpkg.com/text-e...@0.6.4/lib/encoding.js"></script>
Agniva De Sarker (Gerrit)
Patch Set 11:
Patch Set 11:
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.
I'm happy to do this, I just want confirmation first!
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:
<!-- Polyfill for TextEncoder and TextDecoder -->
<script src="https://unpkg.com/text-e...@0.6.4/lib/encoding.js"></script>
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 -
if (!window.TextEncoder || !window.TextDecoder) {
let s = document.createElement('script');
s.src = "https://unpkg.com/text-e...@0.6.4/lib/encoding.js"
document.body.appendChild(s)
}
Richard Musiol (Gerrit)
Patch Set 11:
Patch Set 11:
Patch Set 11:
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.
I'm happy to do this, I just want confirmation first!
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:
<!-- Polyfill for TextEncoder and TextDecoder -->
<script src="https://unpkg.com/text-e...@0.6.4/lib/encoding.js"></script>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 -
if (!window.TextEncoder || !window.TextDecoder) {
let s = document.createElement('script');
s.src = "https://unpkg.com/text-e...@0.6.4/lib/encoding.js"
document.body.appendChild(s)
}
You could use https://cdn.jsdelivr.net/npm/text-e...@0.7.0/lib/encoding.min.js, which is only 5.6 KB on the wire. At this size I would prefer the simpler code.
Agniva De Sarker (Gerrit)
Patch Set 11:
Patch Set 11:
Patch Set 11:
Patch Set 11:
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.
I'm happy to do this, I just want confirmation first!
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:
<!-- Polyfill for TextEncoder and TextDecoder -->
<script src="https://unpkg.com/text-e...@0.6.4/lib/encoding.js"></script>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 -
if (!window.TextEncoder || !window.TextDecoder) {
let s = document.createElement('script');
s.src = "https://unpkg.com/text-e...@0.6.4/lib/encoding.js"
document.body.appendChild(s)
}You could use https://cdn.jsdelivr.net/npm/text-e...@0.7.0/lib/encoding.min.js, which is only 5.6 KB on the wire. At this size I would prefer the simpler code.
SGTM.
Jake Bentvelzen (Gerrit)
Patch Set 11:
Patch Set 11:
Patch Set 11:
Patch Set 11:
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.
I'm happy to do this, I just want confirmation first!
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:
<!-- Polyfill for TextEncoder and TextDecoder -->
<script src="https://unpkg.com/text-e...@0.6.4/lib/encoding.js"></script>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 -
if (!window.TextEncoder || !window.TextDecoder) {
let s = document.createElement('script');
s.src = "https://unpkg.com/text-e...@0.6.4/lib/encoding.js"
document.body.appendChild(s)
}You could use https://cdn.jsdelivr.net/npm/text-e...@0.7.0/lib/encoding.min.js, which is only 5.6 KB on the wire. At this size I would prefer the simpler code.
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>
<!--
Copyright 2018 The Go Authors. All rights reserved.
Use of this source code is governed by a BSD-style
license that can be found in the LICENSE file.
-->
<html>
<head>
<meta charset="utf-8">
<title>Go wasm</title>
</head>
<body>
<script>
let go;
let mod, inst;
if (!WebAssembly.instantiateStreaming) { // polyfill
WebAssembly.instantiateStreaming = async (resp, importObject) => {
const source = await (await resp).arrayBuffer();
return await WebAssembly.instantiate(source, importObject);
};
}
// Add polyfill for TextEncoder and TextDecoder for Microsoft Edge 17/18 support
// https://caniuse.com/#feat=textencoder
if (!window.TextEncoder || !window.TextDecoder) {
let s = document.createElement('script');
s.src = "https://unpkg.com/text-e...@0.6.4/lib/encoding.js"
s.onload = function() {
polyfillsInitialized();
};
document.head.appendChild(s)
} else {
polyfillsInitialized();
}
function polyfillsInitialized() {
let s = document.createElement('script');
s.src = "wasm_exec.js"
s.onload = function() {
main();
};
document.head.appendChild(s)
}
function main() {
go = new Go();
WebAssembly.instantiateStreaming(fetch("test.wasm"), go.importObject).then((result) => {
mod = result.module;
inst = result.instance;
document.getElementById("runButton").disabled = false;
});
}
async function run() {
console.clear();
await go.run(inst);
inst = await WebAssembly.instantiate(mod, go.importObject); // reset instance
}
</script>
<button onClick="run();" id="runButton" disabled>Run</button>
</body>
</html>
Agniva De Sarker (Gerrit)
Richard meant to include https://cdn.jsdelivr.net/npm/text-e...@0.7.0/lib/encoding.min.js directly in a script tag. Then you don't need any additional complexity.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #12 to this change.
misc/wasm: add polyfill for TextEncoder/TextDecoder for Edge support
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: 07a8bfe9901b8418c45cefb1dca446bcadebd1de
GitHub-Pull-Request: golang/go#27296
---
M misc/wasm/wasm_exec.html
1 file changed, 1 insertion(+), 0 deletions(-)
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #13 to this change.
misc/wasm: add polyfill for TextEncoder/TextDecoder for Edge support
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: 00722c49307b404a45b943a9fdf504e6db1d39d5
GitHub-Pull-Request: golang/go#27296
---
M misc/wasm/wasm_exec.html
1 file changed, 5 insertions(+), 0 deletions(-)
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Jake Bentvelzen (Gerrit)
Patch Set 11:
Richard meant to include https://cdn.jsdelivr.net/npm/text-e...@0.7.0/lib/encoding.min.js directly in a script tag. Then you don't need any additional complexity.
Alright sounds good to me!
I gave it a test in Edge+Chrome and pushed it up!
Richard Musiol (Gerrit)
Patch set 13:Code-Review +1
1 comment:
File misc/wasm/wasm_exec.html:
Patch Set #13, Line 17:
// https://caniuse.com/#feat=textencoderI'd remove the unnecessary // and maybe also the "Add", because this currently sounds a bit like a TODO comment.
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #14 to this change.
misc/wasm: add polyfill for TextEncoder/TextDecoder for Edge support
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: a587edae2806e1ca9b6be1c5dfd8824568373bdb
GitHub-Pull-Request: golang/go#27296
---
M misc/wasm/wasm_exec.html
1 file changed, 5 insertions(+), 0 deletions(-)
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Agniva De Sarker (Gerrit)
Patch set 14:Code-Review +1
Jake Bentvelzen (Gerrit)
1 comment:
File misc/wasm/wasm_exec.html:
Patch Set #13, Line 17:
https://caniuse.com/#feat=textencoderI'd remove the unnecessary // and maybe also the "Add", because this currently sounds a bit like a T […]
Agreed.
- Removed trailing space on that comment opening line
- Removed unnecessary //
- Removed word "Add" from start of the sentence.
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Richard Musiol (Gerrit)
Patch set 14:Code-Review +2
Richard Musiol (Gerrit)
Richard Musiol merged this change.
misc/wasm: add polyfill for TextEncoder/TextDecoder for Edge support
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: a587edae2806e1ca9b6be1c5dfd8824568373bdb
GitHub-Pull-Request: golang/go#27296
Reviewed-on: https://go-review.googlesource.com/131718
Reviewed-by: Agniva De Sarker <agniva.qu...@gmail.com>
Reviewed-by: Richard Musiol <neel...@gmail.com>
---
M misc/wasm/wasm_exec.html
1 file changed, 5 insertions(+), 0 deletions(-)
diff --git a/misc/wasm/wasm_exec.html b/misc/wasm/wasm_exec.html
index f5e21e4..7ccdf0a 100644
--- a/misc/wasm/wasm_exec.html
+++ b/misc/wasm/wasm_exec.html
@@ -12,6 +12,11 @@
</head>
<body>
+ <!--
+ Polyfill for TextEncoder and TextDecoder for Microsoft Edge 17/18 support
+ https://caniuse.com/#feat=textencoder
+ -->
+ <script src="https://cdn.jsdelivr.net/npm/text-e...@0.7.0/lib/encoding.min.js"></script>
<script src="wasm_exec.js"></script>
<script>
if (!WebAssembly.instantiateStreaming) { // polyfill
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Agniva De Sarker (Gerrit)
Patch Set 14: Code-Review+2
Richard - This is a candidate for backport https://github.com/golang/go/issues/27393. Should we ?
Richard Musiol (Gerrit)
I'm not sure what our policy should be. The general policy on https://github.com/golang/go/wiki/MinorReleases says:
Our default decision should always be to not backport, but fixes for regressions, security issues, and serious problems with no workaround are backported to the two supported major releases.
One might say that having the latest of a modern browser crash on our wasm example is bad for adoption, so fixing this sooner than later is important. @Brad?
Brad Fitzpatrick (Gerrit)
1 comment:
File misc/wasm/wasm_exec.html:
Patch Set #15, Line 19:
<script src="https://cdn.jsdelivr.net/npm/text-e...@0.7.0/lib/encoding.min.js"></script>Sorry, this isn't acceptable.
We can't be doing unconditional third-party network requests for all users.
When I saw some of the discussion flying by I assumed this was going to be a fallback for old browsers, not for all.
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Richard Musiol (Gerrit)
1 comment:
Patch Set #15, Line 19:
<script src="https://cdn.jsdelivr.net/npm/text-e...@0.7.0/lib/encoding.min.js"></script>
Sorry, this isn't acceptable. […]
What exactly is the issue? Is it about third-party code that could change? If yes, then we could include a hash with the "integrity" attribute.
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Jake Bentvelzen (Gerrit)
Patch Set 15:
(1 comment)
1 comment:
Patch Set #15, Line 19:
<script src="https://cdn.jsdelivr.net/npm/text-e...@0.7.0/lib/encoding.min.js"></script>
What exactly is the issue? Is it about third-party code that could change? If yes, then we could inc […]
FYI, incase simply adding an integrity attribute isn't enough, we can do the more verbose "only load polyfills if needed". I've kept the code I put together here:
https://github.com/silbinarywolf/go/blob/fix-wasm-exec-for-microsoft-edge-optional-polyfill/misc/wasm/wasm_exec.html
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Richard Musiol (Gerrit)
1 comment:
Patch Set #15, Line 19:
<script src="https://cdn.jsdelivr.net/npm/text-e...@0.7.0/lib/encoding.min.js"></script>
FYI, incase simply adding an integrity attribute isn't enough, we can do the more verbose "only load […]
In that case we could make it simpler by putting the polyfill detection in a separate <script> tag and have it append the polyfill script tag conditionally. Browsers execute all <script> tags in order. However, I'd like to only do this if it solves the concrete issue. I'm not yet fully clear on what the issue is.
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
1 comment:
Patch Set #15, Line 19:
<script src="https://cdn.jsdelivr.net/npm/text-e...@0.7.0/lib/encoding.min.js"></script>
In that case we could make it simpler by putting the polyfill detection in a separate <script> tag a […]
Two main reasons it's not acceptable:
1) it adds a new dependency on a third-party service (jsdelivr.net)
2) it makes all users pay the additional bandwidth+latency cost, even when it's not necessaryI care about (2) much more than (1).
If (2) was solved, then (1) would go away over time: eventually browsers will support whatever you're polyfilling and we'd never need to hit that service.
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
Brad Fitzpatrick has created a revert of this change.
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Richard Musiol (Gerrit)
1 comment:
Patch Set #15, Line 19:
<script src="https://cdn.jsdelivr.net/npm/text-e...@0.7.0/lib/encoding.min.js"></script>
Two main reasons it's not acceptable: […]
To me, wasm_exec.html is an example on how to embed Go in a web page. It is not a library that people include. For an example, I prefer simplicity.
The overhead of 5.6 KB is very tiny compared to the size of the wasm binary that the Go compiler currently produces (premature optimization?). It also doesn't affect latency, since script tags get fetched concurrently. Eventually I would simply remove the polyfill from the example when it is not necessary any more. If you have a site and you want to support older browsers, you have to test and polyfill anyways. This wouldn't be the only polyfill that you need.
I'm not yet convinced that this needs a change, but I'm also not strongly opposed to a change.
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
1 comment:
Patch Set #15, Line 19:
<script src="https://cdn.jsdelivr.net/npm/text-e...@0.7.0/lib/encoding.min.js"></script>
To me, wasm_exec.html is an example on how to embed Go in a web page. […]
If it's just an example and you don't think most users will copy/paste this file without really reading it (as I assume), then move the script tag into the comment too. Then only users who read the comment can choose to support old browsers with the added latency hit.
I don't see why this script tag won't block execution: how does the browser know that this script tag won't write to the document body? There's no "defer" attribute or whatever here.
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Agniva De Sarker (Gerrit)
Patch Set 15:
(1 comment)
1 comment:
Patch Set #15, Line 19:
<script src="https://cdn.jsdelivr.net/npm/text-e...@0.7.0/lib/encoding.min.js"></script>
If it's just an example and you don't think most users will copy/paste this file without really read […]
I am fine with moving it to a comment.
Regarding blocking execution, I think what Richard meant was that other scripts get downloaded concurrently, but executed serially. So, it blocks execution, but not download.
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Richard Musiol (Gerrit)
1 comment:
Patch Set #15, Line 19:
<script src="https://cdn.jsdelivr.net/npm/text-e...@0.7.0/lib/encoding.min.js"></script>
I am fine with moving it to a comment. […]
Exactly, it gets downloaded concurrently with wasm_exec.js and then the browser delays execution until all script tags above have been executed.
The comment solution also sounds fine to me.
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
1 comment:
Patch Set #15, Line 19:
<script src="https://cdn.jsdelivr.net/npm/text-e...@0.7.0/lib/encoding.min.js"></script>
Exactly, it gets downloaded concurrently with wasm_exec. […]
I did the revert with the assumption that you wanted to cherry-pick this back to the Go 1.11 branch where we'd only want a single commit, not a series of commits with fixes.
If we don't need this in Go 1.11 now (because it'd be a no-op comment anyway), then we can just roll forward with commenting it out on master.
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.
Richard Musiol (Gerrit)
1 comment:
Patch Set #15, Line 19:
<script src="https://cdn.jsdelivr.net/npm/text-e...@0.7.0/lib/encoding.min.js"></script>
I did the revert with the assumption that you wanted to cherry-pick this back to the Go 1. […]
The point about the cherry-pick is a good one. I've prepared https://go-review.googlesource.com/c/go/+/139037. I think backporting might still be a good idea, because Edge support is probably a common issue right now. Filippo asked for a backport.
To view, visit change 131718. To unsubscribe, or for help writing mail filters, visit settings.