Inject CSS loading with Rollup

[benmccann]

This has rollup inject css loading into the js files instead of putting it into manifest-client and having the application load it from there. Thanks to Rich for writing most of this code!

[Conduitry]

As of bd02d86 I'm seeing lots of missing styles on svelte.dev in production builds when using this.

[Conduitry]

For posterity: It was discovered that what is going on in production builds is that Terser is removing the /* ___foo___ */ comments, which is breaking the handling of styles. A suggestion was to use unknown global variables for these placeholders instead. A hacky but seemingly working implementation of this is:

diff --git a/src/core/create_compilers/RollupCompiler.ts b/src/core/create_compilers/RollupCompiler.ts
index 806dd7c..5939a0e 100644
--- a/src/core/create_compilers/RollupCompiler.ts
+++ b/src/core/create_compilers/RollupCompiler.ts
@@ -127,14 +127,14 @@ export default class RollupCompiler {
 					entry_point = inputs[0].file;
 				}
 			},
-			renderChunk(code: string, chunk: RenderedChunk) {	
+			renderChunk(code: string, chunk: RenderedChunk) {
 				that.chunks.push(chunk);
 			},
 			renderDynamicImport({ format, moduleId, targetModuleId, customResolution }) {
 				if (targetModuleId) {
 					return {
 						left: 'Promise.all([import(',
-						right: `), /* ___${targetModuleId}___ */]).then(x => x[0])`
+						right: `), ___SAPPER___${Buffer.from(targetModuleId).toString('hex')}___]).then(x => x[0])`
 					};
 				} else {
 					return {
@@ -176,7 +176,8 @@ export default class RollupCompiler {
 					let chunk_has_css = false;
 
 					if (chunk.code) {
-						chunk.code = chunk.code.replace(/\/\* ___(.+?)___ \*\//g, (m, id) => {
+						chunk.code = chunk.code.replace(/___SAPPER___([0-9a-f]+)___/g, (m, id) => {
+							id = Buffer.from(id, 'hex').toString();
 							const target = <OutputChunk>Object.values(bundle).find(c => (<OutputChunk>c).facadeModuleId === id);
 
 							if (target) {
@@ -190,7 +191,7 @@ export default class RollupCompiler {
 								}
 							}
 
-							return m;
+							return '';
 						});
 
 						if (chunk_has_css) {

edit: Fixed an issue where it was leaving the unknown global in the code if a chunk didn't have styles to inject.

[Conduitry]

My attention was drawn here because it was the line involved in the fix for Windows - and the new version of it makes me even more uncomfortable than the old version.

Why are we looking at JSON.stringify(mod.input)? Are there multiple fields within mod.input where this filename could occur? And is there a more precise regex we could use here, like /[\\/]client[\\/]client\.\w+\.js$/ or something?

edit: That regex won't work as-is if we continue to JSON.stringify() because it won't catch the escaped backslashes, but I'm hoping that's something we can avoid doing.

[benmccann]

yeah, this whole section is a bit icky. it wasn't the "TODO this is hacky" comment that brought your attention here? 😄

ok, I just updated it to be a bit better

I hope that we can refactor this out into an external plugin (either make it part of rollup-plugin-css-chunks or a new one) very shortly which would make this go away

[Conduitry]

I'm trying to tidy this up a little bit by having inject_styles.mjs be a file that lives in runtime/internal/ and is copied over to src/node_modules/@sapper/internal/ and is imported from @sapper/internal/inject_styles (rather than manually generating and emitting it during the bundling) but I'm running into a couple of weird test failures. Is this something that you'd already explored and then rejected?

[benmccann]

I think the CSS code splitting stuff should be refactored out into an external Rollup plugin. Possibly made part of rollup-plugin-css-chunks (the author of that plugin seems open to the idea). I think that's much easier to do with the code how it currently is than if we were to move it