WordPress / performance

Performance plugin from the WordPress Performance Group, which is a collection of standalone performance modules.
https://wordpress.org/plugins/performance-lab/
GNU General Public License v2.0
369 stars 101 forks source link

Entirety of speculation rules should be filterable, not just the paths to exclude #1156

Open westonruter opened 7 months ago

westonruter commented 7 months ago

Feature Description

Currently the Speculative Loading plugin provides a plsr_speculation_rules_href_exclude_paths filter to exclude URLs (and URL patterns) from being speculatively loaded. However, two situations came up recently where this was not sufficient.

  1. In #1140, WooCommerce was found to create non-idempotent add-to-cart links which add products to cart when prefetched/prerendered. WooCommerce seems to try to get around the non-idempotency by adding rel=nofollow to the links. However, there is no way to exclude links via such an attribute without manually modifying the default rules, which was done in https://github.com/WordPress/performance/pull/1142. (This was to avoid having to add a WooCommerce-specific URLPattern to exclude URLs with an add-to-cart query param, since excluding links with rel=nofollow may be generally advisable: https://github.com/WICG/nav-speculation/issues/309).
  2. In a comment on the aforementioned issue, a user wanted to customize the speculation rules so that specific URLs were opted-in for eager prerendering. That is, instead of specifying a list of URLs/URLPatterns to exclude, they wanted to provide a list of URLs/URLPatterns to include. This is not possible at present either.

To account for these two use cases, I suggest that the entire set of speculation rules (speculation ruleset?) be filterable, doing something like this:

--- a/plugins/speculation-rules/helper.php
+++ b/plugins/speculation-rules/helper.php
@@ -109,5 +109,13 @@ function plsr_get_speculation_rules() {
        );
    }

-   return array( $mode => $rules );
+   $ruleset = array( $mode => $rules );
+
+   /**
+    * Filters the entire speculation ruleset.
+    *
+    * @param array  $ruleset Ruleset.
+    * @param string $mode    Mode used to apply speculative prerendering. Either 'prefetch' or 'prerender'.
+    */
+   return (array) apply_filters( 'plsr_speculation_ruleset', $ruleset, $mode );
 }

Also, @swissspidy suggested in https://github.com/WordPress/performance/issues/1144#issuecomment-2058311409 that a JSON Schema could be written which could validate whatever is being returned by the filter. If not valid, it could trigger a _doing_it_wrong().

westonruter commented 7 months ago

I had thought that only one speculationrules is allowed on page, making it more important to allow the entire single ruleset to be filterable. But @tunetheweb corrected me by pointing out that Multiple speculation rules are indeed allowed. They all get merged together. So this lessens the need for this plugin's speculation rules to be filterable.

masvil commented 7 months ago

It would be great to have the ability to set a limit when using wildcards. For example, eagerly prerender /product/* up to 8. In this case, given the limit of 10, we would still have 2 available on hover "slots" provided by the Moderate setting.

swissspidy commented 7 months ago

Also, @swissspidy suggested in #1144 (comment) that a JSON Schema could be written which could validate whatever is being returned by the filter. If not valid, it could trigger a _doing_it_wrong().

For context, my original use case was validation in unit tests, but runtime validation is interesting too. To make sure that's not too slow, I'd transform the JSON schema into a PHP file and then use that for the validation. Then there's no need to do any JSON parsing.


It would be great to have the ability to set a limit when using wildcards. For example, eagerly prerender /product/* up to 8. In this case, given the limit of 10, we would still have 2 available on hover "slots" provided by the Moderate setting.

This sounds like a feature request for https://github.com/WICG/nav-speculation/issues.

tunetheweb commented 7 months ago

It would be great to have the ability to set a limit when using wildcards. For example, eagerly prerender /product/* up to 8. In this case, given the limit of 10, we would still have 2 available on hover "slots" provided by the Moderate setting.

This sounds like a feature request for https://github.com/WICG/nav-speculation/issues.

These are already separate limits. so you can prerender up to 10 eagerly, and 2 moderately.

masvil commented 7 months ago

These are already separate limits. so you can prerender up to 10 eagerly, and 2 moderately.

Check https://altsetup.com. I have:

image

and

<script type="speculationrules">
{
  "prerender": [{
    "where": {
      "and": [
        { "href_matches": "/*" },
        { "not": {"selector_matches": ".do-not-prerender"}},
        { "not": {"selector_matches": "[rel=nofollow]"}}
      ]
    },
    "eagerness": "eager"
  }]
}
</script>

The 10 eager prerenders happen, but the 2 moderate ones (on hover) don't. Why?

westonruter commented 7 months ago

I believe because Chrome has a limit of 10 prerenders.

masvil commented 7 months ago

I believe because Chrome has a limit of 10 prerenders.

According to @tunetheweb, the limits are 10 eagerly + 2 moderately, and I can confirm it works like that on another site I'm testing it on. The issue is on https://altsetup.com and some other sites having similar stack. I can provide admin access to check it.

EDIT It's like if an eager prerender is not performed ("failure" state) because the initiating page already has too many prerenders ongoing, then it is not a candidate anymore for the moderate prerendering on hover. It happens in case of href_matches": "/*"

westonruter commented 7 months ago

I see Chrome is reporting this error with the prerenders:

Failure - The prerender whose eagerness is "eager" was not performed because the initiating page already has too many prerenders ongoing. Remove other speculation rules with "eager" to enable further prerendering.

I see there is only one URL which is "Not triggered", and that is the Contact page:

image

However, that /contact/ URL was also triggered and failed. So yeah, it seems that prevents it from being attempted.

Did you try excluding /contact/ from the eager rules? I believe this is it:

<script type="speculationrules">
{
  "prerender": [{
    "where": {
      "and": [
        { "href_matches": "/*" },
        { "not": { "href_matches": "/contact/" }},
        { "not": {"selector_matches": ".do-not-prerender"}},
        { "not": {"selector_matches": "[rel=nofollow]"}}
      ]
    },
    "eagerness": "eager"
  }]
}
</script>
masvil commented 7 months ago

I see there is only one URL which is "Not triggered", and that is the Contact page:

Actually all URLs having failure state are not eligible anymore to be triggered on hover. It's more clear if you navigate on any post page, where there are related posts URLs on the bottom. All those are not triggered on hover because excluded by the initial 10 eager prerenders without being excluded.

Did you try excluding /contact/ from the eager rules? I believe this is it:

I just replaced the script with yours, and now /contact/ prerender is triggered on hover, as you can see.

tunetheweb commented 7 months ago

This is basically an extension of this bug: https://issues.chromium.org/issues/335277576. Once a speculation fails it is not retried.

But to be honest a high eagerness is best used with a targeted list of URLs where you KNOW there is a high probability the link will be used. The lower eagerness (moderate or conservative) is for URLs where this cannot be predicted.

The scattergun approach of trying to eagerly fetch ALL URLs, with the low eagerness backup for the same URLs is not recommended. Speculating 12 URLs per page load is a LOT of potential wastage which will affect both your hosting costs (every visitor is now effectively 12 visitors) and, more importantly, your visitors bandwidth and CPU resources. This is precisely why we have this 10 URL limit. And it be honest I think that's a little high and may be lowered if we see a lot of this scattergun approach.

I would suggest a more targeted approach for eagerly fetched URLs, or just using less eagerly fetched URLs.