-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Load storage polyfill conditionally #27619
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
Load storage polyfill conditionally #27619
Conversation
Hi @krzksz. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
51a3163
to
cba9d46
Compare
@krzksz looks like it will be not easy to cover these changes with automatic tests. Do you think it's possible to cover your changes with some tests? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @krzksz,
Your changes looks good to me.
Could you fix the static tests?
As we discussed in Slack - it's too hard to cover these changes with automated tests, so I put label "not required" |
Hi @ihor-sviziev, thank you for the review. |
@magento run Functional Tests CE |
@magento create issue |
@magento run all tests |
Hi @krzksz, thank you for your contribution! |
Hi @krzksz! Thanks for this PR. This is a small, but very helpful fix. One thing, I see that this polyfill been used only in frontend area(declared in frontend layout), but you added a condition in base require.js. This means that this polyfill will appear for the admin area as well. Could you please move this condition in frontend requirejs-config file? |
@omiroshnichenko done in #28749 |
Description (*)
This PR is a part of my effort to improve Magento 2 frontend performance.
This one follows the approach from #27618 which let's us remove loading of storage polyfill so it is only applied for those rare cases when it is not supported. I also reformatted
requirejs-config.js
keys a bit so it is aligned with other configurations you can find in Magento codebase.Manual testing scenarios (*)
localStorage
orsessionStorage
are not available.Contribution checklist (*)
Resolved issues: