Make it work with Webpack 5, use webpack-virtual-modules as a dependency

[non25]

Using webpack-virtual-modules as a dependency has the benefit of easier maintenance in contrary to copypasting.
The downside is that users are required to make slight changes to their webpack.config.js.
The message existing users will recieve on update trying to compile without SveltePlugin is quite straighforward.
What do you guys think ? :)

[syvb]

This approach was considered before at #59 (comment). This would probably be a better approach than #136, since this doesn't involve trying to do some weird compiler hooking here.

[syvb]

This is a breaking change, the package.json should have a major version bump to 3.0.0.

[non25]

This approach was considered before at #59 (comment).

Rich Harris: and someone upgrading an existing project to this version of svelte-loader would see a very unhelpful and cryptic error message from webpack-virtual-modules.

The only thing he mentioned is that we would get unhelpful and cryptic error message, which is easier to fix than trying to put wvm in the loader, and continually fix problems which arise from that.

[syvb]

These changes should be documented in the CHANGELOG.md. I sent a PR (to your fork), non25#1, to document the changes since the last release.

[syvb]

I just tested this on a project that uses svelte-loader, and this works perfectly with Webpack 5 and emitCss. Thanks for getting this working!

[non25]

@Smittyvb have you tested changing css in components with emitCss: true ? Are changes reflected ?

[syvb]

@non25 Just tried that, and it appears I have to restart the watch server whenever I change the CSS. It looks like changes to the CSS aren't being automatically picked up and updated

[non25]

@Smittyvb That is sad. Webpack 5 hard-caches virtual modules, sadly.
That should be fixed in webpack-virtual-modules somehow, or we could try something else..

[syvb]

This looks like it might have been a regression in webpack-virtual-modules. It looks like sysgears/webpack-virtual-modules#69 fixed this bug, but the bug has somehow re-appeared.

[non25]

@Smittyvb I feel like it fixed the reload every virtual file on any change bug too much. 😄
The interesting part was that I've made a poor test which told that reloading on any change is still occurs, but in real world dev mode with svelte-loader the result is the opposite.

[syvb]

Luckily, I've found an easy workaround here: instead of doing emitCss: true, do emitCss: mode === "production". This disables emitCss during development (where bundle size doesn't matter), and enables them in production (where you won't be doing any live changes).

Perhaps svelte-loader should check if watch mode is enabled, and ignore emitCss in watch mode?
Edit: An alternative to disabling it in watch mode would be to just document the issue.

[non25]

That is what I'm using right now, but it has important caveat: emitCss: false inserts style tags upon creating component instance, and emitCss: true gets every component css in the import order, which could lead to some style bugs.

Personally, I would document the issues, merge a Webpack 5 compatible PR and work from there. The current state of svelte-loader is sad.

I'm currently in the middle of hacking around virtual-modules free solution, which is based on inlining the css in base64 into compiled component and making some custom loader string looked up here:
webpack/webpack#11074 (comment)

Will post another branch once I manage to get something working.

[non25]

@Smittyvb I would also recommend using svelte-loader-hot with similar PR in the meantime. It has working HMR.
rixo/svelte-loader-hot#10

[syvb]

@non25 Yep, just tested on svelte-loader-hot, the issue is fixed there!

[non25]

So, my friend made it possible with little help from me.

We can drop webpack-virtual-modules altogether and inline css into compiled components in base64.

I think this approach is much better.

• no changes to webpack.config.js are needed
• emitCss: true works in dev mode, changes are reflected and reloads are triggered

The only downside is that webpack devServer log is polluted with base64'd styles after filenames, but I would take that trade anyday. 🙂

Take a look: https://github.com/non25/svelte-loader/tree/emitcss-inline

I think we could close this and open that. 😄

[non25]

There's also another approach: https://github.com/non25/svelte-loader/tree/emitcss-to-map
It makes use of virtualModules Map, containing pairs: *.svelte.css: content which are deleted every time css-loader gets content from them to prevent leaks, if that works that way.
At first glance it does the same as previous without downside of polluting webpack's console with base64 encodes.. 🐤
I dunno what will happen if somebody try to simultaneosly compile SSR or something... 🤷‍♂️

[syvb]

Here's a comparison of the branches:

Branch	master	wvm-as-dep	emitcss-inline	emitcss-to-map
Full Webpack 5 support	No	Yes	Yes	Yes
Unpolluted console	Yes	Yes	No	Yes
emitCss works in dev mode	No	No	Yes?	Yes?
emitCss works in build mode	No	Yes	Yes?	Yes?

[non25]

emitCss works for both emitcss-* branches in dev and build modes, but there's a possible problem of overlapping css storage keys on emitcss-to-map branch when building both SSR and client-rendered builds.
I don't know how to check if this approach breakes something.

The safe solution based on emitcss-to-map branch would be generating some unique key in addition to the css path, and passing it as a parameter to the second loader run too. That way no loader with same component path could accidentally read or overwrite that.

What do you think? 🤔

[syvb]

Just tested the emitcss-to-map branch, and I'm getting ReferenceError: $2 is not defined at runtime only in watch mode. Not getting any such errors with other branches. It's originating from this code:

if (module.hot) {
	const { configure, register, reload } = require('[root]/node_modules/svelte-loader/lib/hot-api.js');

	module.hot.accept();

	if (!module.hot.data) {
		// initial load
		configure({});
		$2 = register("node_modules/svelte-routing/src/Router.svelte", $2);
	} else {
		// hot update
		$2 = reload("node_modules/svelte-routing/src/Router.svelte", $2);
	}
}

export default $2;

emitcss-inline appears to work great though, aside from the console pollution.

[non25]

That is due to hotReload: true, it didn't work for me in master too.
Does HMR work in svelte-loader at all ? 🤔

[non25]

So I've tested emitcss-to-map on both webpack 4 and 5 with emitCss: true in dev and build modes and came to a conclusion that it is the best approach.
Take a look at the latest commit: master...non25:emitcss-to-map
The only thing left would be to fix emitCss: true test, which doesn't work after I edit the css to match the new import string. 🤷‍♂️

[benmccann]

That is due to hotReload: true, it didn't work for me in master too.
Does HMR work in svelte-loader at all ? thinking

@rixo is our resident HMR expert if you have questions about getting it working

[non25]

@benmccann
I talked to him. I'm using his patched svelte-loader-hot in production with webpack 5. Will make a PR there too.
Would be nice to merge svelte-loader-hot improvements into this svelte-loader, because it is official and people are more likely to use it.
What do you think ?

[benmccann]

Without really knowing what's involved, I'd say that sounds reasonable

[non25]

@benmccann master...non25:emitcss-to-map
What do you think about this branch ?
It fixes virtual modules reloading / not reloading problem once and for all and makes it work with webpack 5.
Who are going to review my proposed changes ? 🤔

[benmccann]

looks nice! I don't know this code base super well, but I like that you're supporting an extra version of webpack with less code! I'd say go ahead and send it as a PR. (fyi, I merged #149 already so you can rebase against that. and also it's probably better not to bump the version number because we'll do that during the release process)

[non25]

Closing in favor of: #151