-
Notifications
You must be signed in to change notification settings - Fork 384
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
Integrate RewriteAmpUrls transformer from toolbox library #6037
Integrate RewriteAmpUrls transformer from toolbox library #6037
Conversation
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.
I haven't yet looked into this in depth, but I'm seeing AMP validation errors:
For scripts, I'm seeing this:
<script async nomodule src="https://cdn.ampproject.org/v0.js"></script>
<script src="https://cdn.ampproject.org/v0/amp-analytics-0.1.mjs" async="" custom-element="amp-analytics" type="module" crossorigin="anonymous"></script>
<script src="https://cdn.ampproject.org/v0/amp-bind-0.1.mjs" async="" custom-element="amp-bind" type="module" crossorigin="anonymous"></script>
<script src="https://cdn.ampproject.org/v0/amp-form-0.1.mjs" async="" custom-element="amp-form" type="module" crossorigin="anonymous"></script>
<script src="https://cdn.ampproject.org/v0/amp-install-serviceworker-0.1.mjs" async="" custom-element="amp-install-serviceworker" type="module" crossorigin="anonymous"></script>
<script src="https://cdn.ampproject.org/v0/amp-lightbox-0.1.mjs" async="" custom-element="amp-lightbox" type="module" crossorigin="anonymous"></script>
<script src="https://cdn.ampproject.org/v0/amp-mustache-0.2.mjs" async="" custom-template="amp-mustache" type="module" crossorigin="anonymous"></script>
<!-- dns-prefetch links, style[amp-custom], various other link elements... -->
<script async="" src="https://cdn.ampproject.org/v0.mjs" type="module" crossorigin="anonymous"></script>
It seems:
- The
nomodule
versions of the AMP component scripts are missing, except for the AMP runtime. - The
v0.mjs
script is not located in the same spot as thev0.js
script is located. I'd expect them to appear next to each other.
Yes, that's why the PR is currently still in draft. Some component seems to strip the |
The problem of the missing nomodules was due to the ordering of the default transformers, where However, this made me aware that the |
Codecov Report
@@ Coverage Diff @@
## develop #6037 +/- ##
=============================================
- Coverage 75.39% 75.38% -0.01%
Complexity 5706 5706
=============================================
Files 218 218
Lines 17270 17265 -5
=============================================
- Hits 13021 13016 -5
Misses 4249 4249
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Can't wait to leverage this in the Web Stories plugin as well 😻 |
Note that I've changed the description to make it so it doesn't actually fix two issues:
|
80cf883
to
71f26ea
Compare
Plugin builds for f78bcab are ready 🛎️!
|
Summary
This PR fetches the new changes from the toolbox library and integrates them with the plugin.
The ES Modules can be tested and profiled with the following plugin: https://gist.github.com/schlessera/4127ab30f4235f1acbc11e3eee77701a
Dependency for #4600
Dependency for #5581
Related #5378
Depends on:
To opt-in to LTS versioning for AMP scripts, the following plugin code can be used:
Checklist