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

fix(remix-react): avoid duplicate loader calls when using prefetch-intent #2938

Merged
merged 5 commits into from
May 16, 2022

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Apr 20, 2022

This fixes a nuanced issue when using <Link prefetch="intent" /> due to prefetch cache behavior. Subsequent <link rel="prefetch"> elements to the same locations as previous prefetch elements will not leverage prior prefetch cache entries unless the endpoint returned a cache-control header. Couldn't find much official docs on this, but GoogleChromeLabs/quicklink#21 (comment) was the most relevant bit I found.

So, in the scenario described by #1249 when a loader does not return cache-control, the behavior ended up being:

  1. Hover on /users/1
    • Adds prefetch links to call the loader for /users/1 which has no cache-control header
  2. Navigate to /users/1
    • Standard loader calls use the prefetch cache entry for /users/1
    • Prefetch link elements are removed since it's now an active route
  3. Navigate to /users/2
    • Prefetch links were being added back for /users/1 since it was still in a shouldPrefetch state from it's prior hover and is no longer active
    • These subsequent prefetch links would not use the pre-existing prefetch cache and without cache-control would cause another hit to the server for the now inactive route.

This PR updates it such that shouldPrefetch is unset upon navigating to the link in question, thus requiring another hover/focus intent to re-add <link rel="prefetch"> elements. If developers wish to avoid subsequent hits to their loaders on those calls they should leverage a cache-control header which will be respected by the prefetch links.

Closes #1249

  • Tests

@brophdawg11 brophdawg11 force-pushed the brophdawg11/prefetch-dup-loader branch from d65159d to 23a5719 Compare May 2, 2022 15:42
@@ -185,7 +185,29 @@ test.describe("prefetch=intent (hover)", () => {
"#nav link[rel='modulepreload'][href^='/build/routes/without-loader-']",
{ state: "attached" }
);
expect(await page.locator("#nav link").count()).toBe(3);
expect(await page.locator("#nav link").count()).toBe(1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryanflorence Now that we setShouldPrefetch(false) in cancelIntent these tests are invalid since the link tags remove - just confirming that's expected 👍

@MichaelDeBoey MichaelDeBoey changed the title fix: Avoid dup loader calls when using prefetch-intent fix(remix-react): avoid duplicate loader calls when using prefetch-intent May 9, 2022
@ryanflorence ryanflorence merged commit 03b86cc into dev May 16, 2022
@ryanflorence ryanflorence deleted the brophdawg11/prefetch-dup-loader branch May 16, 2022 20:45
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-9e5cd6d-20220517 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v1.5.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: redundant loader invocation when using prefetch="intent"
3 participants