Augmented chunk hashes based on imported css code #1665

dionysiusmarquis · 2020-12-09T15:16:55Z

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

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.

src/core/create_compilers/RollupCompiler.ts

benmccann · 2020-12-10T16:29:06Z

src/core/create_compilers/RollupCompiler.ts

+		const css_hashing = () => {
+			const css_hashes: Record<string, string> = {};
+			return <Plugin>{
+				name: 'sapper-css-hashing',


Can this plugin be combined with the sapper-css-injection plugin? It seems like the hashing is really necessary because of what that plugin is doing and isn't something we'd use in a standalone fashion

It needs to be before the css chunk plugin and is also critical for the client chunk.

🤔 Actually this exact issue should also apply to rollup-plugin-css-chunks as there is also some chunk.code stuff happening without a corresponding augmentChunkHash hook. So sapper-css-hashing should actually fix this for the css chunk plugin as well as the sapper rollup compiler.

I was hoping to combine the sapper-css-injection plugin with rollup-plugin-css-chunk: domingues/rollup-plugin-css-chunks#8. There's no reason that our css injection functionality couldn't be used by other Rollup projects, so I think it'd better live as a project separate from Sapper

Thinking about it this maybe could have been solved on rollup-plugin-css-chunks level. I already tried to update to the newest version, but that had a major update in the mean time. Using the newest version in sapper had noticeable side effects. Fix it there might still need extra work on sapper side, I guess.

I just sent #1666 to update to the latest rollup-plugin-css-chunks. I had to bump Rollup as well to make it work

src/core/create_compilers/RollupCompiler.ts

benmccann · 2020-12-23T21:16:58Z

It looks like this will need to be rebased. I sent a small PR in #1677 and didn't realize it would conflict with this

dionysiusmarquis · 2021-02-21T14:34:53Z

@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 · 2021-03-26T02:47:13Z

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 · 2021-04-07T15:02:37Z

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 · 2023-01-11T16:36:01Z

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