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

Symbol should not be polyfilled on Chrome 49 (neither on Chrome 42 and 38) #1161

Closed
wants to merge 4 commits into from

Conversation

Nickinthebox
Copy link

@Nickinthebox Nickinthebox commented Feb 19, 2022

On Chrome 49 (windows XP), we have an error: TypeError: Cannot redefine property: replace if our polyfill.io URL include the polyfill of Symbol.

Chrome 49 is the last available Chrome on Windows XP / Vista and Mac OS 10.6-10.8 (x64)
Chrome 42 is the last available Chrome on Android 4
Chrome 38 is the last available Chrome on Mac OS 10.6 (I1-32)

I checked on https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol and fixed some limits in the code for Symbol as well as Symbol.iterator and Symbol.unscopables.

However, according to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/replace , Symbol.replace (which seems to cause the error) is only supported from version 50... so I have let <50 for the Symbol.replace config... I hope the error will disappear with this PR (not tested).

@Nickinthebox Nickinthebox requested a review from a team as a code owner February 19, 2022 22:36
@CLAassistant
Copy link

CLAassistant commented Feb 19, 2022

CLA assistant check
All committers have signed the CLA.

@origamiserviceuser origamiserviceuser added this to Backlog in Origami ✨ Feb 19, 2022
@github-actions github-actions bot added the library Relates to an Origami library label Feb 19, 2022
@JakeChampion JakeChampion self-assigned this Feb 19, 2022
@JakeChampion
Copy link
Owner

Hi @Nickinthebox - thanks for the pull-request, the tests for this are running at https://github.com/Financial-Times/polyfill-library/actions/runs/1875430851

@@ -27,7 +27,7 @@ edge_mob = "<13"
ie = "*"
ie_mob = "*"
safari = "<9"
chrome = "<49"
chrome = "<38"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nickinthebox have you ran the tests for this polyfill on chrome 38 and seen that they pass? The same question applies to the other polyfills in the pull-request as well.

If not, I can do that sometime this week 👍

@JakeChampion
Copy link
Owner

@Nickinthebox The tests for Chrome 42 and lower are failing on this pull-request - https://github.com/Financial-Times/polyfill-library/runs/5272168306?check_suite_focus=true

Would you be able to look into resolving those at all?

@Nickinthebox
Copy link
Author

This is weird because the fail of tests are on other polyfill.

For example, in the logs I see:

2022-02-21T10:13:11.5367221Z Failures:�[39m�[22m
2022-02-21T10:13:11.5370279Z  - chrome/38.0:
2022-02-21T10:13:11.5370939Z     -> AbortController passes the feature detect
2022-02-21T10:13:11.5372056Z        http://bs-local.com:9876/test?includePolyfills=yes&always=no&feature=AbortController
2022-02-21T10:13:11.5372596Z        false == true
2022-02-21T10:13:11.5372994Z  - chrome/38.0:
2022-02-21T10:13:11.5373508Z     -> AbortController AbortSignal is a function
2022-02-21T10:13:11.5374228Z        http://bs-local.com:9876/test?includePolyfills=yes&always=no&feature=AbortController
2022-02-21T10:13:11.5374746Z        AbortSignal is not defined

Which is about AbordSignal which is supposed to be polyfilled on Chrome 38:
https://github.com/Financial-Times/polyfill-library/blob/master/polyfills/AbortController/config.toml

And this is not changed by my PR... 🤷

@JakeChampion
Copy link
Owner

This is weird because the fail of tests are on other polyfill.

For example, in the logs I see:

2022-02-21T10:13:11.5367221Z Failures:�[39m�[22m
2022-02-21T10:13:11.5370279Z  - chrome/38.0:
2022-02-21T10:13:11.5370939Z     -> AbortController passes the feature detect
2022-02-21T10:13:11.5372056Z        http://bs-local.com:9876/test?includePolyfills=yes&always=no&feature=AbortController
2022-02-21T10:13:11.5372596Z        false == true
2022-02-21T10:13:11.5372994Z  - chrome/38.0:
2022-02-21T10:13:11.5373508Z     -> AbortController AbortSignal is a function
2022-02-21T10:13:11.5374228Z        http://bs-local.com:9876/test?includePolyfills=yes&always=no&feature=AbortController
2022-02-21T10:13:11.5374746Z        AbortSignal is not defined

Which is about AbordSignal which is supposed to be polyfilled on Chrome 38: https://github.com/Financial-Times/polyfill-library/blob/master/polyfills/AbortController/config.toml

And this is not changed by my PR... 🤷

This is likely because AbortSignal depends on Symbol, which was changed in this pull-request

@JakeChampion
Copy link
Owner

JakeChampion commented May 6, 2023

/ok-to-test sha=e0be16b

running tests at https://github.com/Financial-Times/polyfill-library/actions/runs/4902362724

@romainmenke
Copy link
Collaborator

romainmenke commented May 7, 2023

@Nickinthebox I've tried to reproduce the issue you are facing but I wasn't able to.
From your comment I understand that the issue is happening only in Chrome 49?

If you are using polyfill.io you shouldn't be getting the Symbol polyfill in Chrome 49.
Lowering the version constraints further won't change much.

In chrome 49 :

https://polyfill.io/v3/polyfill.min.js?features=Symbol.replace
-> only Symbol.replace

https://polyfill.io/v3/polyfill.min.js?features=Symbol
-> no polyfills


Is it possible your project is also using core-js and is effectively having two polyfills for Symbol.replace? One from core-js and one from polyfill.io

@Nickinthebox
Copy link
Author

In deed we also use core-js! I will dig into this. Sorry for the noise.

@robertboulton robertboulton removed this from Backlog in Origami ✨ Jul 26, 2023
@JakeChampion
Copy link
Owner

In deed we also use core-js! I will dig into this. Sorry for the noise.

@Nickinthebox and/or @romainmenke should we be closing this PR?

@mhassan1
Copy link
Collaborator

Closed as not needed

@mhassan1 mhassan1 closed this Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
library Relates to an Origami library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants