sveltejs / sapper

The next small thing in web development, powered by Svelte
https://sapper.svelte.dev
MIT License
6.99k stars 433 forks source link

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

Closed ehrencrona closed 3 years ago

ehrencrona commented 3 years ago

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

Tests

benmccann commented 3 years ago

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

ehrencrona commented 3 years ago

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 commented 3 years ago

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.