-
Notifications
You must be signed in to change notification settings - Fork 9.4k
[Backport + Bugfix] Move polyfill JavaScript code to be included as a remote script from the head block #17033
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
[Backport + Bugfix] Move polyfill JavaScript code to be included as a remote script from the head block #17033
Conversation
|
Hi @ihor-sviziev. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
…the head block. - Add block back - Drop original content from polyfill.phtml
…the head block. - Add copyright and description to polyfill.js file
fed37e7 to
c9f67f2
Compare
|
Hi @ihor-sviziev, thank for collaboration. I think we can remove |
|
Hi @VladimirZaets, However we found an issue with current implementation in Safari and Firefox with disabled localstorage support. We're getting an error "Error: Attempted to assign to readonly property.". After this issue all another JS files (including magento components) just doesn't work. Related issues (with localstorage polyfills): mortzdk/localStorage#1 https://gist.github.com/orion110217/ef26968000df7290de03a8f774b46b79#gistcomment-2316288 Looks like fix will be much harder than I thought. PS: We were able to reproduce issue #16568 by disabling localstorage in Firefox and applying this patch. |
|
@magento-engcom-team give me 2.2.5 instance |
|
Hi @ihor-sviziev. Thank you for your request. I'm working on Magento 2.2.5 instance for you |
|
Hi @ihor-sviziev, here is your Magento instance. |
|
I found really good steps to reproduce. Described them in #17195 |
|
We had long discussion with @VladimirZaets in slack and decided that following changes can't fix the issue. We found following things:
I'm closing this PR as it's not fixing real issue |
Description
I investigated issue described in #16568 and found that we have 2 different polyfill implementaions for localStorage and sessionStorage:
These implementation are fully different. 1st one works good, but 2nd one adds strange behavior - adds separate cookie with
ls_prefix in name for each item that should be stored in localstorage andss_<some_id>_for session storage. As result - when customer goes through website - it will add new and new cookies (because of this strangess_<some_id>_prefix).For some reasons sometimes, probably because of race conditions, we're having 2nd implementation used instead of 1st one.
This PR moving 1st implementation into head section (before requirejs), so it will be loaded first for sure.
Also it doing partial backport of #14318 to
2.2-developbranch in order not to add backward incompatible changes.Fixed Issues (if relevant)
Related Pull Requests
Manual testing scenarios
Contribution checklist