Skip to content

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

Merged
sebastianbenz merged 1 commit intoampproject:mainfrom
erwinmombay:modulepreload
Mar 25, 2021
Merged

when using module/nomodule make sure to use modulepreload instead of preload for the priority hint#1161
sebastianbenz merged 1 commit intoampproject:mainfrom
erwinmombay:modulepreload

Conversation

@erwinmombay
Copy link
Copy Markdown
Member

@erwinmombay erwinmombay commented Mar 16, 2021

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Member Author

erwinmombay commented Mar 16, 2021

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

@honeybadgerdontcare
Copy link
Copy Markdown

honeybadgerdontcare commented Mar 16, 2021

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

@erwinmombay
Copy link
Copy Markdown
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
Copy Markdown

honeybadgerdontcare commented Mar 16, 2021

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
Copy Markdown
Collaborator

sebastianbenz commented Mar 16, 2021

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

@sebastianbenz
Copy link
Copy Markdown
Collaborator

sebastianbenz commented Mar 25, 2021

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

@sebastianbenz sebastianbenz sebastianbenz approved these changes

+2 more reviewers

@jridgewell jridgewell jridgewell left review comments

@honeybadgerdontcare honeybadgerdontcare honeybadgerdontcare left review comments

Reviewers whose approvals may not affect merge requirements

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