-
Notifications
You must be signed in to change notification settings - Fork 136
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
structuredClone
polyfill is broken
#1269
Comments
config.toml is also not fully correct. [browsers]
android = "*"
bb = "*"
chrome = "<98"
edge = "*"
edge_mob = "*"
firefox = "<94"
firefox_mob = "<94"
ie = "*"
ie_mob = "*"
opera = "<84"
op_mob = "<68"
op_mini = "*"
safari = "<15.4"
ios_saf = "<15.4"
samsung_mob = "<18.0" |
@romainmenke I don't have any problems trying to fix that, but I don't know how to debug on chrome 48... How could I install that specific version on Windows? |
Thank you for the speedy reply @CheloXL :) We use https://www.browserstack.com to test everything. I am not sure if they have a free plan or if there is a comparable service that does have a free plan :/ |
I will see if I can use the free tier, but from what I saw, I only have 1 minute to try to debug... not very useful. I already fixed the config.toml in my branch. Will let you know if I can fix the error. |
@romainmenke Ok, I think I fixed the issue. Pull request |
@CheloXL Thank you for that! Unfortunately it still fails at the same point when I run the tests. I tried to debug it but the original polyfill doesn't have any comments to explain the purpose of parts and is written in a style that isn't the most readable for me. Reverting this would take the pressure of :) |
@romainmenke Please do the revert. I would like to fix any issues, but I'm not sure how to continue. I already have a VM with chrome 48 installed and was able to reproduce the original issue (RegEx has no .flags property so I had to check individual flags g, i, m). After that fix, everything was working fine. As for you trying to debug the polyfill: Did you try to debug the original one (the one from ungap) or the one I re-wrote? You can't use the original one as it uses modern js features (for..of, arrow functions, etc) so it would not work at all. Can you point me where the test fails now? (line# of the failing test). Maybe now there is another problem. |
The tests in CI don't get that far, they already fail on the checks for
It is unexpected that this Chrome 48 is able to get past the test for
Trying the one you re-wrote, but I also briefly looked at the original.
fails here : proclaim.equal(deserialized['null'], null); Also want to thank you again for submitting the polyfill in the first place and for working so hard on a fix :) |
Ok... this is strange. I was able to test in browserstack (online). Tested both in Chrome/Windows and Firefox/Windows. Chrome version 48/47/46. Firefox 34/33/32. Only found an issue in Firefox (that version doesn't has string.includes, so I replaced it with indexOf). But after that, everything passes. |
But I found that by adding a few log statements with |
mmm. The test for that is Or: The polyfill uses both Map/Set. Maybe the tests are including them even if Chrome 48 doesn't need them, and that's the problem? Can you run the above file and see if you see the same error? The only thing that I can think that could be a problem is the name of the field in the object (that it is "null"). Maybe you can try to change that in the test ( Sorry to bother you with this, but I don't have a way to reproduce this. Again, running the above file through BrowserStack produces the right output. |
I don't know why yet, but when I remove this bit it doesn't error : var bi = null;
if ("BigInt" in window && typeof BigInt == "function"){
try {
bi = BigInt(1);
} catch (e) {
//no BigInt support.
}
} If I remove that and all references to
Have you been able to get the local dev server running with |
/* eslint-env mocha, browser */
/* globals proclaim, structuredClone */
describe('structuredClone', function () {
it('is a function', function () {
proclaim.isFunction(structuredClone);
});
it('has correct arity', function () {
proclaim.arity(structuredClone, 1);
});
var bi = null;
var obj = {
bigint: bi,
"null": null
};
var deserialized = structuredClone(obj);
console.log(deserialized);
it('serializes values', function () {
/* eslint-disable-next-line dot-notation */
proclaim.equal(deserialized['null'], null);
});
}); logs : |
This comment was marked as resolved.
This comment was marked as resolved.
The map is to preserve references when cloning. And yes, without the map, you are cloning the same object to the infinity and beyond... :) |
Ok. Found the problem: I'm not sure why, but on Chrome 48, the tests are loading AND executing the Map/Set polyfills. As I guessed, the problem is in there, and specifically with the keys I don't know why the Map/Set polyfills are beign loaded and are replacing their native counterparts. If I run the test manually (as shown on the file attached above, where no Map/Set PF are included), it works as expected. Also running the tests from the local url server on my latest Chrome version works fine (Map/Set PF are included, but they do not replace native Map/Set). So, after changing the field name from Now it should be working (at least all tests passes). |
That is how the polyfill library works :)
Is there a way to make this test pass with I also want to know and understand why this is happening. Without fully understanding this I don't feel comfortable shipping this polyfill. |
I don't think so, as the problem is in the Map polyfill.
This is happening because the pf uses a Map to store references to values, so on deserialization it is able to preserve those references. When serializing, it first stores the key It is not a problem on the polyfill, and I think it is a really contrived use case of Map (using So, basically the problem is the following: var map = new Map();
map.set('null', 1);
map.set(null, 2);
console.log(map.get(null) === 2);// should be true, but with Map PF returns false. |
Thank you for this! I will take a look at this and try to find a fix. |
Pretty much all keywords have this issue. |
Probably because of the underlining implementation of the Map. I didn't check, but I guess it is using an object to store the key/value pairs, and since an object only accepts strings as keys, that's where the problem is. But again, I'm just guessing... |
exactly that is happening yeah.. // If this is just a primitive, we can cast it to a string and return it
return ''+recordKey; So there is no difference between |
I think this will resolve the issues : #1275 Thank you for providing all this extra info 🙇 |
Issue is fixed, thank you for working with me on this one @CheloXL 🙇 |
@romainmenke Thanks to you for your great support!!! |
see : https://github.com/Financial-Times/polyfill-library/actions/runs/3981951365
pr : #1242
@CheloXL @JakeChampion It seems that there are some differences between browser versions in serialize/deserialize.
In Chrome 48 for example one part is a string and the other is true
![Screenshot 2023-01-29 at 18 10 14](https://user-images.githubusercontent.com/11521496/215343445-9b83f13a-fcf2-469c-8b0b-80f3f9aa3e22.png)
null
:Does anyone have time to resolve these issues?
If not, we should revert these changes until someone does have time.
The text was updated successfully, but these errors were encountered: