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.

Sign up for GitHub

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

Jump to bottom

when using module/nomodule make sure to use modulepreload instead of preload for the priority hint #1161

Merged
merged 1 commit into from
Mar 25, 2021
Merged

when using module/nomodule make sure to use modulepreload instead of preload for the priority hint #1161

merged 1 commit into from
Mar 25, 2021

Conversation

erwinmombay
Copy link
Member

Currently chrome double downloads the module (mjs) script twice when we use preload instead of modulepreload in Chrome. Safari does not support modulepreload unfortunately but this is the tradeoff we are making

@CLAassistant
Copy link

CLAassistant commented Mar 16, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

sebastianbenz
sebastianbenz approved these changes Mar 16, 2021
View reviewed changes
Copy link
Collaborator

@sebastianbenz sebastianbenz left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! How long does it take for the validator changes to go live?

@erwinmombay
Copy link
Member Author

i believe @honeybadgerdontcare is working on the validator changes so he'll have a more accurate timeline than i can estimate.

@honeybadgerdontcare
Copy link

It's in cl/363067094 and will be live this week or early next.

@erwinmombay
Copy link
Member Author

erwinmombay commented Mar 16, 2021 •
edited
Loading

this will be temporary and we will most likely need to revert back to preload at some point, should we leave this PR as is or have some sort of mechanism in place to toggle this in the optimizer?

@honeybadgerdontcare
Copy link

Temporary is between now and earliest May. Even then some users may not upgrade to the latest, correct? In that case it may be a bit longer, but I would think this could be reverted instead of toggled when that version of Chrome is widely adopted.

It'll continue to be supported in the validator regardless.

@sebastianbenz
Copy link
Collaborator

OK, I'll merge this PR once validator changes are live. We can revert behaviour in a subsequent release.

@sebastianbenz
Copy link
Collaborator

Just verified in the AMP Validator that it's now valid AMP.

@sebastianbenz sebastianbenz merged commit 88f372f into ampproject:main Mar 25, 2021
@westonruter westonruter mentioned this pull request Mar 30, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jridgewell jridgewell jridgewell left review comments

@sebastianbenz sebastianbenz sebastianbenz approved these changes

@honeybadgerdontcare honeybadgerdontcare honeybadgerdontcare left review comments

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@erwinmombay @CLAassistant @honeybadgerdontcare @sebastianbenz @jridgewell