sapper icon indicating copy to clipboard operation
sapper copied to clipboard

Augmented chunk hashes based on imported css code

Open dionysiusmarquis opened this issue 4 years ago • 5 comments

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

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

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.

dionysiusmarquis avatar Dec 09 '20 15:12 dionysiusmarquis

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

benmccann avatar Dec 23 '20 21:12 benmccann

@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 avatar Feb 21 '21 10:02 dionysiusmarquis

@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.

dionysiusmarquis avatar Feb 21 '21 14:02 dionysiusmarquis

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

benmccann avatar Mar 26 '21 02:03 benmccann

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.

dionysiusmarquis avatar Apr 07 '21 15:04 dionysiusmarquis

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

benmccann avatar Jan 11 '23 16:01 benmccann