Move Speculation Rules to new location as purely a standalone plugin #946
Conversation
…in subdirectories.
Co-authored-by: Weston Ruter <[email protected]>
Implement new module to use Speculation Rules API for prerendering documents on hover
Update feature branch with `trunk`
…or defensive coding, mostly relevant for tests where setting is not registered).
There was a problem hiding this comment.
@westonruter @swissspidy @mukeshpanchal27 This is pretty much ready now and refreshed for the way things work now in feature/modules-to-plugins.
We should probably merge #1002 first, then do another quick refresh of this one, then merge this one.
After that we'll have both auto-sizes and speculation-rules in the feature/modules-to-plugins branch and can continue development against that branch until it's merged into trunk.
| run: npm run test-php -- -- -- --testsuite auto-sizes | ||
| - name: Running multisite unit tests for Auto-sizes for Lazy-Loaded Images Plugin | ||
| run: npm run test-php-multisite -- -- -- --testsuite auto-sizes | ||
| - name: Running single site unit tests for Speculation Rules Plugin | ||
| run: npm run test-php -- -- -- --testsuite speculation-rules | ||
| - name: Running multisite unit tests for Speculation Rules Plugin | ||
| run: npm run test-php-multisite -- -- -- --testsuite speculation-rules |
There was a problem hiding this comment.
This is cumbersome so we should probably add shortcuts via scripts in composer.json and/or package.json.
There should also be a shortcut that runs the single site and multisite tests respectively for all of these plugins at once, so that we don't have to add every single plugin to this GitHub workflow file.
Let's handle that in a separate PR though, not directly related to the Speculation Rules work.
There was a problem hiding this comment.
I opened #1017 for that.
phpcs.xml.dist
Outdated
| @@ -8,7 +8,7 @@ | |||
| <rule ref="WordPress-Docs"/> | |||
| <rule ref="WordPress-Extra" /> | |||
| <rule ref="WordPress.WP.I18n"/> | |||
| <config name="text_domain" value="performance-lab,default,auto-sizes"/> | |||
| <config name="text_domain" value="performance-lab,default,auto-sizes,speculation-rules"/> | |||
There was a problem hiding this comment.
This will need to be adjusted once #1002 is merged, instead a plugins/speculation-rules/phpcs.xml.dist file should be added then.
Let's merge it then. 🚀 |
Merged! |
…les-standalone-plugin
|
@westonruter @swissspidy @mukeshpanchal27 Should be ready for merge now! You had already approved prior, but since I now changed the base branch, it would be great if you could do another quick sanity check. Note that all the actual PHP code for Speculation Rules is unchanged and was already approved in the |
felixarntz
requested review from
westonruter,
swissspidy and
mukeshpanchal27
|
@westonruter It looks like a test related to the CDATA fix you implemented fails, see https://github.com/WordPress/performance/actions/runs/8071362067/job/22050829990?pr=946. Maybe it is because the test code isn't limited to the specific WordPress versions? Since the CDATA issue is only a problem on WordPress 6.4, we probably need to limit the test accordingly. Either we make those assertions conditional, or we mark the entire test to skip unless it's WordPress version 6.4.x. WDYT? On another note: I think the code comment linking to the Trac ticket points to the wrong ticket? Shouldn't it be https://core.trac.wordpress.org/ticket/60320? |
|
@felixarntz I opted to keep the test but add a condition for the WP version. Tests are passing now. |
tests/plugins/speculation-rules/plsr-url-pattern-prefixer-test.php
Outdated
Show resolved
Hide resolved
tests/plugins/speculation-rules/speculation-rules-helper-test.php
Outdated
Show resolved
Hide resolved
tests/plugins/speculation-rules/speculation-rules-settings-test.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Weston Ruter <[email protected]>
| * Class 'PLSR_URL_Pattern_Prefixer'. | ||
| * | ||
| * @package speculation-rules | ||
| * @since n.e.x.t |
There was a problem hiding this comment.
What about version? When we change it from n.e.x.t to actual version 1.0.0
There was a problem hiding this comment.
Since the plugin is already published, this should be already 1.0.0, no?
plugins/speculation-rules/load.php
Outdated
| <?php | ||
| /** | ||
| * Plugin Name: Speculation Rules | ||
| * Plugin URI: https://github.com/WordPress/performance/tree/trunk/modules/js-and-css/speculation-rules |
There was a problem hiding this comment.
| * Plugin URI: https://github.com/WordPress/performance/tree/trunk/modules/js-and-css/speculation-rules | |
| * Plugin URI: https://github.com/WordPress/performance/tree/trunk/plugins/speculation-rules |
There was a problem hiding this comment.
Good catch! I fixed this for auto-sizes as well.
|
@swissspidy @mukeshpanchal27 I've addressed your feedback! This is probably good to merge now. |
Summary
See #904 (comment): Since the Speculation Rules feature will not be published as a module (given the unbundling of Performance Lab) and thus only as a plugin, it should be moved to the new location for standalone plugins (see #934).
This PR is still against the feature branch and prepares that feature branch for a clean merge after #934 has been implemented. There's no blocker for merging this into the feature branch though.
No functional changes are included as part of this PR.
Checklist
[Focus]orInfrastructurelabel.[Type]label.no milestonelabel.