-
Notifications
You must be signed in to change notification settings - Fork 131
Add Speculative Loading opt-in for authenticated requests #2097
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
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk #2097 +/- ##
==========================================
+ Coverage 67.11% 67.22% +0.11%
==========================================
Files 93 93
Lines 7732 7758 +26
==========================================
+ Hits 5189 5215 +26
Misses 2543 2543
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: swissspidy <[email protected]>
… add/authenticated-speculative-loading
Co-authored-by: swissspidy <[email protected]>
|
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.
@westonruter Looks great! A few notes that ideally should be addressed before merge.
plugins/speculation-rules/hooks.php
Outdated
return ( | ||
( | ||
! is_user_logged_in() | ||
|| | ||
'any' === $option['authentication'] | ||
|| | ||
( current_user_can( 'manage_options' ) && 'logged_out_and_admins' === $option['authentication'] ) | ||
) | ||
&& | ||
( | ||
(bool) get_option( 'permalink_structure' ) | ||
|| | ||
/** |
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.
For readability, it would be more accessible to have separate conditions and early return true
or return false
instead of this giant single clause.
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.
IMO, this seems more readable to me. It would also facilitate eventual filtering per #2097 (comment)
Otherwise, is this what you have in mind?
// Disabled if the user is logged in, unless the setting explicitly allows the current user's role.
if (
is_user_logged_in()
&&
'any' !== $option['authentication']
&&
( ! current_user_can( 'manage_options' ) || 'logged_out_and_admins' !== $option['authentication'] )
) {
return false;
}
// Disabled if pretty permalinks are not enabled, unless explicitly overridden by the filter.
if (
! (bool) get_option( 'permalink_structure' )
&&
/**
* Filters whether speculative loading should be enabled even though the site does not use pretty permalinks.
*
* Since query parameters are commonly used by plugins for dynamic behavior that can change state, ideally any
* such URLs are excluded from speculative loading. If the site does not use pretty permalinks though, they are
* impossible to recognize. Therefore, speculative loading is disabled by default for those sites.
*
* For site owners of sites without pretty permalinks that are certain their site is not using such a pattern,
* this filter can be used to still enable speculative loading at their own risk.
*
* @since 1.4.0
*
* @param bool $enabled Whether speculative loading is enabled even without pretty permalinks.
*/
! apply_filters( 'plsr_enabled_without_pretty_permalinks', false )
) {
return false;
}
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.
Yeah. Separate clauses are easier to follow I think, as you don't have to look at a large multi-nested clause and figure out where the ||
and &&
are :)
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.
Done in 48b9016
plugins/speculation-rules/hooks.php
Outdated
$option = plsr_get_stored_setting_value(); | ||
return ( | ||
( | ||
! is_user_logged_in() |
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.
We may want to explicitly include 'logged_out' === $option['authentication']
next to this. Makes it easier to understand why the ! is_user_logged_in()
, plus it also makes it safer e.g. in case we add further possible options in the future there would be no chance to mess up this check here by accident.
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.
@felixarntz I'm not entirely clear on this. Can you provide a suggestion, especially after 48b9016?
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.
This actually looks okay as is now.
'description' => __( 'The eagerness setting defines the heuristics based on which the loading is triggered. "Eager" will have the minimum delay to start speculative loads, "Conservative" increases the chance that only URLs the user actually navigates to are loaded.', 'speculation-rules' ), | ||
'type' => 'string', | ||
'enum' => array_keys( plsr_get_eagerness_labels() ), | ||
), | ||
'authentication' => array( | ||
'description' => __( 'Only unauthenticated pages are typically served from cache. So in order to reduce load on the server, speculative loading is not enabled by default for logged-in users. If your server can handle the additional load, you can opt in to speculative loading for all logged-in users or just administrator users only.', 'speculation-rules' ), |
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.
Worth clarifying here that this would still only affect the frontend I think. Enabling something for logged-in users could easily be misinterpreted as enabling it in WP Admin, which is not what this does.
Also for WP Admin there would be the problems of "action links" (activating plugins by accident etc.), so this would give confidence that it's not a relevant problem for opting in.
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.
Like so?
diff --git a/plugins/speculation-rules/settings.php b/plugins/speculation-rules/settings.php
index a9789b75..39fbd48f 100644
--- a/plugins/speculation-rules/settings.php
+++ b/plugins/speculation-rules/settings.php
@@ -163,7 +163,7 @@ function plsr_register_setting(): void {
'enum' => array_keys( plsr_get_eagerness_labels() ),
),
'authentication' => array(
- 'description' => __( 'Only unauthenticated pages are typically served from cache. So in order to reduce load on the server, speculative loading is not enabled by default for logged-in users. If your server can handle the additional load, you can opt in to speculative loading for all logged-in users or just administrator users only.', 'speculation-rules' ),
+ 'description' => __( 'Only unauthenticated pages are typically served from cache. So in order to reduce load on the server, speculative loading is not enabled by default for logged-in users. If your server can handle the additional load, you can opt in to speculative loading for all logged-in users or just administrator users only. This only applies to pages on frontend; admin screens are excluded.', 'speculation-rules' ),
'type' => 'string',
'enum' => array_keys( plsr_get_authentication_labels() ),
),
@@ -211,7 +211,7 @@ function plsr_add_setting_ui(): void {
),
'authentication' => array(
'title' => __( 'User Authentication Status', 'speculation-rules' ),
- 'description' => __( 'Only unauthenticated pages are typically served from cache. So in order to reduce load on the server, speculative loading is not enabled by default for logged-in users. If your server can handle the additional load, you can opt in to speculative loading for all logged-in users or just administrator users only.', 'speculation-rules' ),
+ 'description' => __( 'Only unauthenticated pages are typically served from cache. So in order to reduce load on the server, speculative loading is not enabled by default for logged-in users. If your server can handle the additional load, you can opt in to speculative loading for all logged-in users or just administrator users only. This only applies to pages on frontend; admin screens are excluded.', 'speculation-rules' ),
),
);
foreach ( $fields as $slug => $args ) {
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.
See also e1f3c18
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.
This sounds right. Maybe say "admin screens remain excluded"? To clarify that this only working on the frontend is already the default behavior, and also (currently) the only behavior. But not sure about that, either way works.
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.
Done in 4add0f4
… add/authenticated-speculative-loading
Co-authored-by: felixarntz <[email protected]>
Co-authored-by: felixarntz <[email protected]>
Speculative Loading was omitted from logged-in users per #1157:
See also #1157 (comment):
Nevertheless, if the server is able to handle the load of additional dynamic non-cacheable requests, there should be an opt-in to allow users to enable Speculative Loading for logged-in users. For sites where users are primarily logged-in (for example e-commerce, forums, social, membership) there should already be infrastructure in place to scale for non-cached responses. Users of such sites should be able to benefit from Speculative Loading as well. In fact, Speculative Loading is even more necessary for such sites since uncached requests involve more latency.
Being able to opt in to Speculative Loading would allow the administrators to experience the site more closely to what regular visitors experience, without first having to log out. In order to facilitate this specifically, the opt-in can limit to just administrators in addition to unauthenticated visitors. (Maybe the plugin should even default to enabling Speculative Loading for administrators, so they are able to see the impact of the plugin after it is activated.)
This is related to the No-cache BFCache plugin which allows for instant back/forward navigations to be enabled for authenticated pages. With this PR, authenticated pages also can leverage Speculative Loading. This greatly improves the experience of a logged-in user browsing around a site, perhaps the single user who browses the site the most: the administrator!
This adds a new option to the settings screen:
Adding an such an option to the plugin does not violate the 80% principle since it's not targeting core.
To Do