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

Hoist AMP script preload for minor performance improvement 🛥️ #7184

Merged

Conversation

sanjaiyan-dev
Copy link
Contributor

Hoist AMP script preload for minor performance improvement.

Sorry if I made any mistakes :(

@changeset-bot
Copy link

changeset-bot bot commented Oct 8, 2022

🦋 Changeset detected

Latest commit: d96d8bf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/amp Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -1,7 +1,7 @@
// https://amp.dev/documentation/guides-and-tutorials/learn/spec/amp-boilerplate/
const boilerplate = `
<link rel="preload" as="script" href="https://cdn.ampproject.org/v0.js" />
Copy link
Member

Choose a reason for hiding this comment

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

this should use a tab instead of spaces to align with the other lines

@benmccann
Copy link
Member

I can't believe this would make any difference. However, if we were going to make this change, wouldn't you also want to move up <script async src="https://cdn.ampproject.org/v0.js"></script>?

@Rich-Harris
Copy link
Member

It does seem doubtful that it would make any difference, and it's very surprising to me that the <link> is worth the extra bytes when the <script> immediately follows it, but this does in fact match the current recommended AMP boilerplate, which appears to have changed since this was first created.

@sanjaiyan-dev
Copy link
Contributor Author

I can't believe this would make any difference. However, if we were going to make this change, wouldn't you also want to move up <script async src="https://cdn.ampproject.org/v0.js"></script>?

Sorry, actually I thought this would have very minor performance improvement because it would have least small improvement than current implementation of adding link:rel=preload just before the script tag.

@sanjaiyan-dev
Copy link
Contributor Author

It does seem doubtful that it would make any difference, and it's very surprising to me that the <link> is worth the extra bytes when the <script> immediately follows it, but this does in fact match the current recommended AMP boilerplate, which appears to have changed since this was first created.

Ok, I actually made these changes after viewing them on next js implementation of AMP.
They had it like-:

  1. link preload
  2. Boilerplate CSS
  3. Script tag

@Rich-Harris Rich-Harris merged commit 4bb7453 into sveltejs:master Oct 10, 2022
@Rich-Harris
Copy link
Member

thanks!

@sanjaiyan-dev
Copy link
Contributor Author

thanks!

🤝💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants