sveltejs / sapper

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

Augmented chunk hashes based on imported css code #1665

Closed dionysiusmarquis closed 1 year ago

dionysiusmarquis commented 3 years ago

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

Tests

Fixes: #1660

Currently the chunk hashes don't include "injected" css code coming from script css imports. This is because the injection is happening in bundle hooks and at this stage all depending chunk imports will already be resolved using hashes that don't "react" to css changes. This is especially crucial in caching environments. To ensure that the chunk hashes also change as soon as imported css gets updated this PR adds a css_hashing plugin to the rollup compiler. The augmentChunkHash hook is utilised to provide a hash based on all imported css codes.

benmccann commented 3 years ago

It looks like this will need to be rebased. I sent a small PR in https://github.com/sveltejs/sapper/pull/1677 and didn't realize it would conflict with this

dionysiusmarquis commented 3 years ago

@benmccann This approach doesn't work anymore, since the update of rollup-plugin-css-chunks (https://github.com/sveltejs/sapper/commit/8a2769da8ed95c3daacfadb645946fc9822be4b1#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R63).

That's because the css files won't be present in thechunk.modules anymore. This is most likely because of this change: https://github.com/domingues/rollup-plugin-css-chunks/commit/3632de47adea7a0447e0d53b0a608cecac1307c8#diff-dcdc3e0b3362edb8fec2a51d3fa51f8fb8af8f70247e06d9887fa934834c9122L90

Following: https://github.com/rollup/rollup/issues/3655

dionysiusmarquis commented 3 years ago

@benmccann Ok, I rebased and updated the approach. Maybe you could have another look. I had to add moduleSideEffects: 'no-treeshake'. I was also able to simplify the augmentChunkHash part because I had to determine is_svelte_css in transform already.

benmccann commented 3 years ago

It looks like there's a merge conflict here. This one was pretty complicated to understand and test. I wonder if it might be better to just make sure this works as intended in SvelteKit instead? SvelteKit uses Vite for CSS bundling which is a completely different set of code, so there's a good chance this issue doesn't exist there

dionysiusmarquis commented 3 years ago

Everything is working as expected in SvelteKit, I'd say. Running dev imported css inside <script> will be added "unbundled" as inline css. build will put them in the corresponding bundled css, correctly hashed.

benmccann commented 1 year ago

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