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

Feat: Implement speculative prerendering #13616

Draft
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

Swanand01
Copy link
Collaborator

@Swanand01 Swanand01 commented Apr 3, 2024

Summary

This PR makes use of the Speculation Rules API to enable dynamically prerendering editor and story URLs.

User-facing changes

Prerendering for these URLs has been enabled for the following pages:

  1. Dashboard - Create New Story, Edit story
  2. All Stories - Create New Story, Edit story, View story

Testing Instructions

This PR can be tested by following these steps:

  1. Go to the Stories Dashboard page.
  2. Open the Application Tab and go to Speculative loads > Speculations.
  3. A list of URLs with their Status should appear.
  4. Hover over a story to see the status change to Ready.

WIP

  1. Add prerendering on the front end for story permalink when hovering over the link to view the story
  2. Add a check to see if the Speculation Rules plugin is active

Reviews

Does this PR have a security-related impact?

No.

Does this PR change what data or activity we track or use?

🤔

Does this PR have a legal-related impact?

No.

Checklist

  • This PR addresses an existing issue and I have linked this PR to it
  • I have tested this code to the best of my abilities
  • I have verified accessibility to the best of my abilities (docs)
  • I have verified i18n and l10n (translation, right-to-left layout) to the best of my abilities
  • This code is covered by automated tests (unit, integration, and/or e2e) to verify it works as intended (docs)
  • I have added documentation where necessary
  • I have added a matching Type: XYZ label to the PR

Fixes #13603

Copy link
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing a first draft! This already seems to work quite well and makes for near-instant navigations to single stories or the story editor. 👏 🚀

includes/Speculative_Prerendering.php Outdated Show resolved Hide resolved
includes/Plugin.php Outdated Show resolved Hide resolved
includes/Speculative_Prerendering.php Outdated Show resolved Hide resolved
includes/Speculative_Prerendering.php Outdated Show resolved Hide resolved
includes/Speculative_Prerendering.php Outdated Show resolved Hide resolved
includes/Plugin.php Outdated Show resolved Hide resolved
includes/Speculative_Prerendering.php Outdated Show resolved Hide resolved
@swissspidy swissspidy added Type: Enhancement New feature or improvement of an existing feature Type: Performance Performance related issues and enhancements. labels Apr 4, 2024
@Swanand01
Copy link
Collaborator Author

Hi @swissspidy !
We should also prerender the stories when a stories block is added to the front end. I was wondering what hook I could use for the same, to attach the load_rules method.

@swissspidy
Copy link
Collaborator

We should also prerender the stories when a stories block is added to the front end. I was wondering what hook I could use for the same, to attach the load_rules method.

The right hook to print the rules on the frontend would be wp_footer. Note: we should not print anything on the frontend if the plsr_print_speculation_rules function exists (which means the Speculation Rules plugin already handles this for us).

However, the stories on the frontend all open in a lightbox using amp-story-player, so I don't think it makes sense to add a rule for those, as it won't have an effect (please correct me if I'm wrong). What we could add a rule for is the "View all stories" link that is displayed below the stories.

That said: What I don't like about just hooking into wp_footer is that it means the admin-only Dashboard class (which is a dependency of the Speculative_Rendering class right now) gets loaded on the frontend as well, which is not intended.

That means we would need to design Speculative_Rendering in a slightly different way to decouple admin and frontend. For example, Speculative_Rendering could just print the script tag and have a filterable list of rules, and then all the relevant services or classes would filter the list instead. But not sure I like that, especially since the Renderer classes are not singletons, and ideally we would only add the filter once.
But this way we would make things more compatible with the Speculation Rules plugin in the long term.

➡️ Long story short, maybe let's hold off on any frontend stuff for now and focus on the admin side of things.

@Swanand01
Copy link
Collaborator Author

The quickEdit test is failing here too. Debugged it, and it seems the response returned here is 403.

const [response] = await Promise.all([
page.waitForNavigation(),
page.click(`#${elmId} a[rel="bookmark"]`),
]);

This test logs in as author, but doing the same manually does not return a 403.

@swissspidy WDYT could be going wrong here?

@swissspidy
Copy link
Collaborator

Are speculation rules added on that page and Is the browser perhaps preloading the link? Maybe that causes some issues?
The error says response is null, which is suspicious.

Could be that a simple Puppeteer update fixes this, but we have some more errors on that PR 🙃

@swissspidy
Copy link
Collaborator

I don't like that, because

the Renderer classes are not singletons, and ideally we would only add the filter once.

The Web Stories block can be added dozens of times on a page. Adding the rules in maybe_render_archive_link or any other method there would just print the rules dozens of times as well.

@Swanand01
Copy link
Collaborator Author

Swanand01 commented May 3, 2024

I don't like that, because

the Renderer classes are not singletons, and ideally we would only add the filter once.

The Web Stories block can be added dozens of times on a page. Adding the rules in maybe_render_archive_link or any other method there would just print the rules dozens of times as well.

That makes sense. I'll take a closer look at this. If you have any ideas, please do let us know.

@swissspidy
Copy link
Collaborator

So I think we can actually simplify this a lot address this issue.

We don't actually need to know whether a stories block is on the page or not. We can simply add the rules everywhere on the frontend, that little inline JSON doesn't hurt. And if there's no block, nothing happens as there is nothing to preload.

In Speculation_Rules:

  • Remove the Dashboard injection from the Speculation_Rules service
  • Hook Speculation_Rules::print_rules into admin_footer and wp_footer. print_rules() calls get_rules()
  • No need for get_matches, get_matches_for_pageorload_rules`
  • If there are no rules, it should simply not print anything
  • In Speculation_Rules::get_rules() add an apply_filters() call so other services and third-party plugins can filter the rules (or disable the feature altogether)
  • By default, it would add two rules if ! is_admin(): links to the archive page and any story permalink (currently $view_story_url)

In Dashboard:

  • Use the new filter filter to add rules if currently on the stories page

- Remove the Dashboard injection
- Hook print_rules into admin_footer and wp_footer.
- Remove generate_matches, get_matches_for_page and load_rules
- In get_rules() add an apply_filters() call
@Swanand01
Copy link
Collaborator Author

Okay, strange. I can try to reproduce this later this week.

But let's not have this block us. In the meantime, do you perhaps have any thoughts on #13616 (comment) and how we could decouple the frontend & backend parts here? Don't want to load the admin classes on the frontend obviously.

I have pushed some commits that implement your suggestions. Would you please take a look?
Also, were you able to find out what's wrong with the failing tests?

@swissspidy
Copy link
Collaborator

Also, were you able to find out what's wrong with the failing tests?

Interestingly it seems to pass now with the new version!

.phpstorm.meta.php Outdated Show resolved Hide resolved
'where' => [
'and' => [
[
'href_matches' => [ $edit_story_url, $new_story_url ],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add $view_story_url here too so that links to the story permalink can be preloaded.

We should also test whether $view_story_url should be in prefetch instead, not prerender, given that it's usually loaded in a lightbox anyway.

Copy link
Collaborator Author

@Swanand01 Swanand01 May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested both prefetch and prerender for $view_story_url, and I couldn't see a noticeable difference. Prerender appeared to be just slightly faster, in terms of page loading times.

includes/Admin/Dashboard.php Show resolved Hide resolved
includes/Speculation_Rules.php Show resolved Hide resolved
includes/Speculation_Rules.php Show resolved Hide resolved
tests/phpunit/integration/tests/Speculation_Rules.php Outdated Show resolved Hide resolved
includes/Admin/Dashboard.php Outdated Show resolved Hide resolved
includes/Speculation_Rules.php Outdated Show resolved Hide resolved
includes/Admin/Dashboard.php Outdated Show resolved Hide resolved
includes/Admin/Dashboard.php Outdated Show resolved Hide resolved
@Swanand01
Copy link
Collaborator Author

The E2E tests aren't going past the pendingStories test.

image

@swissspidy @AnuragVasanwala

@swissspidy
Copy link
Collaborator

OK so we have some timeouts apparently? yay fun 🙃

Can you try updating all our Puppeteer/e2e related dependencies (maybe in a new PR)? Just so that the browser we use during tests understands the speculation rules correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement New feature or improvement of an existing feature Type: Performance Performance related issues and enhancements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement speculative prerendering (Speculation Rules)
3 participants