Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Fixes #1667 prefetching fails if clicking quickly back and forth #1668

Merged
merged 5 commits into from Dec 14, 2020

Conversation

ehrencrona
Copy link
Contributor

@ehrencrona ehrencrona commented Dec 11, 2020

Fixes #1667

When prefetching a page that is the current page, no props got stored in prefetching

I'm not entirely sure how to best write a test for this. Any pointers? Managed to add a test. It's a bit brittle (in the sense that if the prefetching logic changes it might pass even if the bug were to be reintroduced, not in the sense of sporadically failing) but I don't see any better way to handle it.

This is the kind of thing that unit tests might be better at, but the routing/preloading logic has so much global state that's pretty much impossible.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

: {};
} else {
preloaded = initial_data.preloaded[i + 1];
else {
Copy link
Member

@benmccann benmccann Dec 11, 2020

Choose a reason for hiding this comment

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

I'm surprised lint isn't catching this, but it should probably be on the previous line: } else {

@benmccann
Copy link
Member

@ehrencrona
Copy link
Contributor Author

Do we need to port this fix to Kit as well? https://github.com/sveltejs/kit/blob/master/packages/kit/src/runtime/internal/renderer/index.js

Good point. Probably, though the code has been rewritten so it's not obvious whether the issue still occurs. I'll try to find time to test that.

@ehrencrona
Copy link
Contributor Author

Do we need to port this fix to Kit as well? https://github.com/sveltejs/kit/blob/master/packages/kit/src/runtime/internal/renderer/index.js

Good point. Probably, though the code has been rewritten so it's not obvious whether the issue still occurs. I'll try to find time to test that.

Update: I ported the test to Kit and it passes without any code changes, so I assume the fix does not need porting. Though I haven't investigated why in detail.

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

Successfully merging this pull request may close these issues.

Following a prefetched link will fail if switching quickly back and forth
2 participants