breaks when used in webpack <=3, with svelte v3

0gust1 commented 5 years ago

Sorry for the annoying issue :-/

TLDR :

It's a cross bug between :

the changes between svelte v2 and svelte v3 compiler strategy in options handling
a mocha test written the old way.
the usage of both this.options and loader-utils.getOptions(this) to handle options. this.options was deprecated in webpack3 and removed in webpack4. loader-utils.getOptions(this) is the right way to do. It appeared in version 1.0.0 (21 Feb 2017) of loader-utils and seems supported since webpack v1

edit: proposed pull request : #90 edit: related issue : #88 edit: possibly related issue/pr : #87

Below, all the analysis I did investigating the issue. Sorry, it's a bit verbose.

Context : Our big project is stuck to webpack 3 at the moment (due to a bug in webpack 4 that will be resolved in webpack 5).

This loader seems to break in webpack3. We have been able to reconstruct a minimal case (using the svelte-app template and making a dichotomy with webpack3 vs webpack4)

with webpack3, the loader breaks at the same line as our big project : let { js, css, warnings } = normalize(compile(processed.toString(), compileOptions));
with webpack4 everything compile fine.

The kind of errors we have :

ERROR in ./src/App.svelte
Module build failed: Error: Error: Error: Unrecognized option 'entry'
    at preprocess.then.catch.err (/ABSOLUTE_PATH/test_svelte3_webpack3/node_modules/svelte-loader/index.js:182:12)
 @ ./src/main.js 1:0-31

On our other project, it's not entry but context.

After further analysis, it seems that the svelte V3 compiler doesn't like the configuration feeded to it when it comes from webpack 3. The validate_options() function of the compiler reject it.

Config is inited here : https://github.com/sveltejs/svelte-loader/blob/b11b4e91fc4ba3f081f243e503c0fb0e424c4784/index.js#L109 where this.options is :

with webpack 3 : the webpack big config object
with webpack 4 : undefined (because it was finally deprecated)

Config is transformed along the way (into a compileOptions object) and, after that, it's used here : https://github.com/sveltejs/svelte-loader/blob/b11b4e91fc4ba3f081f243e503c0fb0e424c4784/index.js#L146

In the minimal test case we have :

compileOptions with webpack 3 and svelte 3

{
  "filename": "/workspace_absolute_path/test_svelte3_webpack3/src/App.svelte",
  "format": "esm",
  "entry": "./src/main.js",
  "module": {
    "rules": [
      { "test": {}, "use": [{ "loader": "svelte-loader", "options": {} }] }
    ],
    "unknownContextRequest": ".",
    "unknownContextRegExp": false,
    "unknownContextRecursive": true,
    "unknownContextCritical": true,
    "exprContextRequest": ".",
    "exprContextRegExp": false,
    "exprContextRecursive": true,
    "exprContextCritical": true,
    "wrappedContextRegExp": {},
    "wrappedContextRecursive": true,
    "wrappedContextCritical": false,
    "strictExportPresence": false,
    "strictThisContextOnImports": false,
    "unsafeCache": true
  },
  "output": {
    "path": "/workspace_absolute_path/test_svelte3_webpack3/public",
    "filename": "bundle.js",
    "chunkFilename": "[id].bundle.js",
    "library": "",
    "hotUpdateFunction": "webpackHotUpdate",
    "jsonpFunction": "webpackJsonp",
    "libraryTarget": "var",
    "sourceMapFilename": "[file].map[query]",
    "hotUpdateChunkFilename": "[id].[hash].hot-update.js",
    "hotUpdateMainFilename": "[hash].hot-update.json",
    "crossOriginLoading": false,
    "jsonpScriptType": "text/javascript",
    "chunkLoadTimeout": 120000,
    "hashFunction": "md5",
    "hashDigest": "hex",
    "hashDigestLength": 20,
    "devtoolLineToLine": false,
    "strictModuleExceptionHandling": false
  },
  "context": "/workspace_absolute_path/test_svelte3_webpack3",
  "devtool": false,
  "cache": true,
  "target": "web",
  "node": {
    "console": false,
    "process": true,
    "global": true,
    "Buffer": true,
    "setImmediate": true,
    "__filename": "mock",
    "__dirname": "mock"
  },
  "performance": {
    "maxAssetSize": 250000,
    "maxEntrypointSize": 250000,
    "hints": false
  },
  "resolve": {
    "unsafeCache": true,
    "modules": ["node_modules"],
    "extensions": [".js", ".json"],
    "mainFiles": ["index"],
    "aliasFields": ["browser"],
    "mainFields": ["browser", "module", "main"],
    "cacheWithContext": false
  },
  "resolveLoader": {
    "unsafeCache": true,
    "mainFields": ["loader", "main"],
    "extensions": [".js", ".json"],
    "mainFiles": ["index"],
    "cacheWithContext": false
  }
}

compileOptions with webpack 4 and svelte 3

{"filename":"/workspace_absolute_path/test_svelte3_webpack3/src/App.svelte","format":"esm"}

0gust1 commented 5 years ago

Corresponding PR : https://github.com/sveltejs/svelte-loader/pull/90

0gust1 commented 5 years ago

Summed up explanation :

in a webpack <=3.x + svelte v3 config, the loader pass a misconfigured option object to the svelte compiler and makes it choke (because the svelte v3 compiler check the content of the option object before using it)
You don't have this problem with webpack >= 4.x + svelte v3, because webpack removed the use of this.options (which now returns undefined), so the final option object is filled with loader-utils.getOptions(this) content only.
You don't have this problem with webpack <= 3.x + svelte v2, because the svelte v2 compiler is less strict about the content of the option object it digests.

I can push the code of the minimal test-case project I used, if needed.

mrkishi commented 5 years ago

Is this.options needed? getOptions() uses this.query and it seems to be supported since webpack v1.

@esarbanis do you remember why you added this.options here?

0gust1 commented 5 years ago

@mrkishi thanks for the precision ! I confess I lacked some energy to dig on the evolution of the API on webpack side. I will do some tests on my minimal test-case. I just hope we won't hit some edge case, trying to get rid of this.options, webpackism is hairy sometimes ^^

0gust1 commented 5 years ago

Some links to webpack loader apis :

this.options is deprecated since webpack 3 and removed in webpack 4 : https://webpack.js.org/api/loaders/#thisoptions
getOptions from loader-utils is the recommended way to go https://github.com/webpack/loader-utils#getoptions

I still don't know why this.options was used, but it worth noting that getOptions has his own sets of bugs in the past (apparently linked to "query paremeters" expressed options).

getOptions appeared in version 1.0.0 (21 Feb 2017) of loader-utils.

I'll make some tests and push changes on my PR today (which should be muuuuch cleaner :) ). Thanks !

0gust1 commented 5 years ago

@mrkishi , @esarbanis : I did some analysis again.

tldr: removing this.options does the job, but breaks a test that maybe should rewritten.

It breaks the should preprocess successfully and should not preprocess successfully
but I tested in my minimal setup (implementing the same preprocess as in the test), and the preprocess works (I can see {width:50px;height:50px} in the preprocessed result and final compiled result).

New test in PR soon.

0gust1 commented 5 years ago

@mrkishi @esarbanis @Rich-Harris @ekhaled @Conduitry

CI is green, everything seems OK. Can you check #90 ?

Rich-Harris commented 5 years ago

Released 2.13.4 with the fix — thanks

sveltejs / svelte-loader

breaks when used in webpack <=3, with svelte v3 #89