Make prefetching styles work more reliably

sveltejs / sapper

The next small thing in web development, powered by Svelte

https://sapper.svelte.dev

MIT License

7k stars 434 forks source link

Make prefetching styles work more reliably #1780

Closed wlach closed 1 year ago

wlach commented 3 years ago

There was a small race between the element being added to the DOM and an onload event being triggered, fix this by making sure we have an onload/onerror handlers set up before adding it to document head. Fixes #1582.

NOTE: All development is currently focused on Sapper's successor, SvelteKit: https://kit.svelte.dev/ We are not adding new features to Sapper. While we might fix some particularly pressing or easy to address bugs, it's highly unlikely that most PRs will be reviewed.

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

[x] 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
[x] This message body should clearly illustrate what problems it solves.
[ ] Ideally, include a test that fails without this PR but passes with it.

Tests

[x] Run the tests with npm test and lint the project with npm run lint

wlach commented 3 years ago

~~(there's something wrong here, it doesn't fix the problem reliably -- back to the drawing board)~~

wlach commented 3 years ago

Ok on further testing I think this does actually work as expected (I had some difficulty setting up a good test environment).

Open the following in a chrome private tab:

https://irydium.netlify.app/

Vs. the following:

https://deploy-preview-36--irydium.netlify.app/

With the first example, navigating to "examples" doesn't always work. I can't reproduce it 100% reliably but I can never reproduce it with the deploy preview (which has this fix applied). It's actually a little unclear to me why we're resolving the stylesheet loads as promises at all-- we can't really take any action if they fail.

wlach commented 3 years ago

Further refined this to handle the case where a CSS element was already loaded (which would result in the promise never resolving). I believe this might be related to this kludge in the repl (I had a similar problem to what this is attempting to fix):

https://github.com/sveltejs/svelte-repl/blob/2d0ecdf288c117f93a208efaaf04f2e38240fb86/src/Repl.svelte#L99

benmccann commented 3 years ago

This would need to be rebased, but is there a reason you don't just use SvelteKit instead? I don't really want to spend time on these edge cases for a deprecated project

wlach commented 3 years ago

This would need to be rebased, but is there a reason you don't just use SvelteKit instead? I don't really want to spend time on these edge cases for a deprecated project

Porting my project to SvelteKit is on my roadmap, though it does some rather esoteric things via rollup that I'm not sure offhand how to do with SvelteKit so I've been holding off on porting things over. I'll probably give it a real shot when SvelteKit hits 1.0.

I'd describe this behaviour more as a bug than an edge case, but I understand if it's not a priority to test and merge. I wound up publishing a fork for myself to hold myself over until I can find the time to port to SvelteKit: https://www.npmjs.com/package/@wlach/sapper

Happy to rebase if that would be enough for you to merge, but I likewise am not so inclined to spend more time on this.

benmccann commented 1 year ago

SvelteKit 1.0 is now out and Sapper is deprecated, so I'm going to close this