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

Integrate RewriteAmpUrls transformer from toolbox library #6037

Merged
merged 11 commits into from
Apr 11, 2021

Conversation

schlessera
Copy link
Collaborator

@schlessera schlessera commented Apr 2, 2021

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:

use AmpProject\Optimizer\Transformer\RewriteAmpUrls;
use AmpProject\Optimizer\Configuration\RewriteAmpUrlsConfiguration;

add_filter( 'amp_optimizer_config', static function ( $config ) {
	$config[ RewriteAmpUrls::class ][ RewriteAmpUrlsConfiguration::LTS ] = true;
	return $config;
} );

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@schlessera schlessera added Performance Optimizer WS:Perf Work stream for Metrics, Performance and Optimizer labels Apr 2, 2021
Copy link
Member

@westonruter westonruter left a 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:

image

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:

  1. The nomodule versions of the AMP component scripts are missing, except for the AMP runtime.
  2. The v0.mjs script is not located in the same spot as the v0.js script is located. I'd expect them to appear next to each other.

@schlessera
Copy link
Collaborator Author

Yes, that's why the PR is currently still in draft. Some component seems to strip the nomodule scripts at some point.

@schlessera
Copy link
Collaborator Author

The problem of the missing nomodules was due to the ordering of the default transformers, where ReorderHead came after RewriteAmpUrls. Switching these in the ordering solved the immediate issue.

However, this made me aware that the ReorderHead transformer uses the URL as array key, so it implicitly deduplicates based on the URL, which was the reason why the nomodule versions went missing. I've created an issue for this, as there might be other instances where multiple <script> elements share a same URL and need to pass through the ReorderHead transformer: ampproject/amp-toolbox-php#125

@codecov
Copy link

codecov bot commented Apr 7, 2021

Codecov Report

Merging #6037 (d70bd74) into develop (81b9df7) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head d70bd74 differs from pull request most recent head f78bcab. Consider uploading reports for the commit f78bcab to get more accurate results
Impacted file tree graph

@@              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              
Flag Coverage Δ Complexity Δ
javascript 80.05% <ø> (ø) 0.00 <ø> (ø)
php 75.19% <100.00%> (-0.01%) 0.00 <0.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
amp.php 0.00% <ø> (ø) 0.00 <0.00> (ø)
includes/class-amp-theme-support.php 86.22% <100.00%> (-0.09%) 245.00 <0.00> (ø)

@swissspidy
Copy link
Collaborator

Can't wait to leverage this in the Web Stories plugin as well 😻

includes/class-amp-theme-support.php Outdated Show resolved Hide resolved
includes/class-amp-theme-support.php Outdated Show resolved Hide resolved
@westonruter westonruter added this to the v2.1 milestone Apr 9, 2021
@westonruter
Copy link
Member

Note that I've changed the description to make it so it doesn't actually fix two issues:

@schlessera schlessera force-pushed the add/rewrite-amp-urls-transformer-from-library branch from 80cf883 to 71f26ea Compare April 10, 2021 15:44
@schlessera schlessera marked this pull request as ready for review April 10, 2021 16:38
@github-actions
Copy link
Contributor

github-actions bot commented Apr 10, 2021

Plugin builds for f78bcab are ready 🛎️!

@westonruter westonruter merged commit 6b1af52 into develop Apr 11, 2021
@westonruter westonruter deleted the add/rewrite-amp-urls-transformer-from-library branch April 11, 2021 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Optimizer Performance WS:Perf Work stream for Metrics, Performance and Optimizer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants