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

AutoExtensionImporter: Append extension scripts after runtime script #1241

Merged

Conversation

schlessera
Copy link
Collaborator

@schlessera schlessera commented Jun 16, 2021

The current implementation of the AutoExtensionImporter transformer adds the needed extension scripts after the viewport node.

However, this creates markup where the head contains all extension scripts first, then the boilerplate style, and then the runtime. In terms of priorities, this seems backwards. Why load the extensions if the runtime itself is not yet registered?

This PR changes the transformer so that it first tries to append the extension scripts after an existing runtime script, and only falls back to using the viewport as a reference node if no runtime script was found.

before:

<meta viewport>
<script extension 1>
<script extension 2>
<script extension 3>
...
<style boilerplate>
<script runtime>

after:

<meta viewport>
<style boilerplate>
<script runtime>
<script extension 1>
<script extension 2>
<script extension 3>
...

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.

This is of course much better! We've been relying on the ReorderHeadTransformer to put things right later. Thanks!

@sebastianbenz sebastianbenz merged commit 5263ab2 into ampproject:main Jun 16, 2021
@schlessera schlessera deleted the fix/append-extensions-after-runtime branch June 16, 2021 19:34
@westonruter
Copy link
Member

first tries to append the extension scripts after an existing runtime script

Aside: As in ampproject/amp-wp#6182 (comment), I'd like to explore appending scripts which are not render critical to the end of the body (e.g. amp-lightbox and amp-analytics).

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

Successfully merging this pull request may close these issues.

3 participants