Downloading a REPL includes some weird HMR stuff

[Rich-Harris]

I just downloaded a REPL example and the main.js included this:

if (module.hot) {
	const { configure, register, reload } = require('/home/travis/build/sveltejs/svelte.technology/node_modules/svelte-loader/lib/hot-api.js');

	module.hot.accept();

	if (!module.hot.data) {
		// initial load
		configure({});
		app = register("src/routes/repl/_components/AppControls.html", app);
	} else {
		// hot update
		app = reload("src/routes/repl/_components/AppControls.html", app);
	}
}

I have no freaking idea where that's coming from.

[Conduitry]

The compiled version of AppControl.html is getting run through the svelte-loader Webpack loader, which is doing a string replacement on it, looking for export defaults. Apparently (I'm thinking because of method hoisting), the first instance of this it sees is the one inside the template string in the download method, and not the one that's the component's real export statement.

So that's a bug in svelte-loader, but kind of an edge case, and there's something else going on. The loader shouldn't even be trying to add hot module reloading to anything when making a production build of the site. The loader is run with the options { css: false, generate: 'ssr', dev: process.env.NODE_ENV === 'development' }. As far as I can tell, dev is not an actual option of the loader. I haven't looked at the loader code enough to see exactly when it allows HMR, but according to the readme these are the criteria. Apparently the 'webpack is minifying code' check isn't working. I remember there being some questions about the value of the NODE_ENV variable, and about the possibility of a build being half-dev and half-prod. I don't know if that's relevant here.

Bluh. I hope this comment is marginally more digestible than the issue I set out to investigate. This doesn't seem to be a simple bug, as there are multiple entities who are probably not doing what they're supposed to be doing.

cc @mrkishi who I believe mentioned the half-dev/half-prod build issue, and cc @ekhaled who implemented svelte-loader's HMR feature.

Once we get this sorted out here, we should also revisit the sapper-template Webpack config, which is also setting a (disregarded?) dev option to svelte-loader for the client build.

[ekhaled]

That's weird as hot loader should not be activated when generate: 'ssr' (based on isServer)

https://github.com/sveltejs/svelte-loader/blob/master/index.js#L114

maybe the config is being ignored somehow?

A surefire way would be to add hotReload: false to the config and see what happens.

[Conduitry]

The code responsible for what's being seen in this issue is not run on the server. The zip file is generated completely on the client side. So that's not the problem.

If I set hotReload: false in this project's webpack config, then yeah it looks like this problem goes away, as expected.

I'm not actually sure where minification is handled - I don't see it mentioned in the config - but apparently svelte-loader isn't picking up on when that's happening.

Is there or was there ever a dev option to svelte-loader? Or is that just a completely incorrect way that this project is calling the loader? Setting the hotReload option to what we're currently setting the dev option to seems like a reasonable way forward. It won't solve the problem in local development of svelte-loader replacing the wrong export default, but it should take care of this happening in the deployed version of the site.

Do you happen to know what conventions there are surrounding when NODE_ENV is set to neither development nor production (for example, if it's not set at all)? This project's webpack config is configuring stuff according to whether it's equal to development, but I see in svelte-loader, it's checking whether it's equal to production. I don't know whether this is related to these issues, but it would be good to know if one way of doing this is preferred when we go to clean some stuff up.

[ekhaled]

There is a dev option in svelte-loader... it gets sent through to the compiler to generate dev output.

However, the hot loader works regardless of whether dev is true or false.

Webpack used to have this.minimize when it was minifying. https://github.com/sveltejs/svelte-loader/blob/master/index.js#L95

I'm not not sure whether it's still the case for webpack 4

[Conduitry]

Pushed a temporary fix to address this on production, but leaving this issue open as there's more to investigate. And, once we're satisfied, we should make the same changes in sapper-template.