From 17acb1000d40b005f2cdb7e4675c75ddad6aa82f Mon Sep 17 00:00:00 2001 From: rjgotten Date: Mon, 22 Jul 2019 15:13:52 +0200 Subject: [PATCH 1/5] feat: hmr error recovery --- src/loader.js | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/src/loader.js b/src/loader.js index a91dd787..6c549210 100644 --- a/src/loader.js +++ b/src/loader.js @@ -31,11 +31,18 @@ function hotLoader(content, context) { locals: !!context.locals, })}); module.hot.dispose(cssReload); + module.hot.accept(function(){}); ${accept} } `; } +function interceptError(callback, interceptor) { + return (err, source) => { + return callback(null, err ? interceptor(err) : source); + }; +} + const exec = (loaderContext, code, filename) => { const module = new NativeModule(filename, loaderContext); @@ -133,17 +140,24 @@ export function pitch(request) { }); }); - const callback = this.async(); + const callback = !options.hmr + ? this.async() + : interceptError(this.async(), (err) => { + let resultSource = `// extracted by ${pluginName}`; + resultSource += hotLoader('', { + context: this.context, + locals: null, + options, + }); + resultSource += `\nthrow new Error(${JSON.stringify(String(err))});`; + return resultSource; + }); childCompiler.runAsChild((err, entries, compilation) => { if (err) { return callback(err); } - if (compilation.errors.length > 0) { - return callback(compilation.errors[0]); - } - compilation.fileDependencies.forEach((dep) => { this.addDependency(dep); }, this); @@ -152,6 +166,10 @@ export function pitch(request) { this.addContextDependency(dep); }, this); + if (compilation.errors.length > 0) { + return callback(compilation.errors[0]); + } + if (!source) { return callback(new Error("Didn't get a result from child compiler")); } From 607532d8883e826143caf6150a982679ebb3e4a6 Mon Sep 17 00:00:00 2001 From: rjgotten Date: Wed, 24 Jul 2019 09:26:25 +0200 Subject: [PATCH 2/5] feat: hmr error recovery test --- test/TestCases.test.js | 18 +++++- test/cases/~hmr-resilience/error-loader.js | 4 ++ test/cases/~hmr-resilience/expected/check.js | 19 ++++++ test/cases/~hmr-resilience/index.css | 3 + test/cases/~hmr-resilience/index.js | 1 + test/cases/~hmr-resilience/webpack.config.js | 62 ++++++++++++++++++++ 6 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 test/cases/~hmr-resilience/error-loader.js create mode 100644 test/cases/~hmr-resilience/expected/check.js create mode 100644 test/cases/~hmr-resilience/index.css create mode 100644 test/cases/~hmr-resilience/index.js create mode 100644 test/cases/~hmr-resilience/webpack.config.js diff --git a/test/TestCases.test.js b/test/TestCases.test.js index caae878e..2f04fe57 100644 --- a/test/TestCases.test.js +++ b/test/TestCases.test.js @@ -29,8 +29,22 @@ describe('TestCases', () => { const casesDirectory = path.resolve(__dirname, 'cases'); const outputDirectory = path.resolve(__dirname, 'js'); + // HMR tests use and render Date.now + let dateNowMock = null; + beforeEach(() => { + dateNowMock = jest + .spyOn(Date, 'now') + .mockImplementation(() => 1479427200000); + }); + + afterEach(() => { + dateNowMock.mockClear(); + }); + for (const directory of fs.readdirSync(casesDirectory)) { if (!/^(\.|_)/.test(directory)) { + const expectsError = /^~/.test(directory); + // eslint-disable-next-line no-loop-func it(`${directory} should compile to the expected result`, (done) => { const directoryForCase = path.resolve(casesDirectory, directory); @@ -59,7 +73,7 @@ describe('TestCases', () => { } webpack(webpackConfig, (err, stats) => { - if (err) { + if (err && !expectsError) { done(err); return; } @@ -76,7 +90,7 @@ describe('TestCases', () => { }) ); - if (stats.hasErrors()) { + if (stats.hasErrors() && !expectsError) { done( new Error( stats.toString({ diff --git a/test/cases/~hmr-resilience/error-loader.js b/test/cases/~hmr-resilience/error-loader.js new file mode 100644 index 00000000..ad8dbf52 --- /dev/null +++ b/test/cases/~hmr-resilience/error-loader.js @@ -0,0 +1,4 @@ +module.exports = function loader() { + const callback = this.async(); + callback(new Error('I am error')); +}; diff --git a/test/cases/~hmr-resilience/expected/check.js b/test/cases/~hmr-resilience/expected/check.js new file mode 100644 index 00000000..b4b6d8e5 --- /dev/null +++ b/test/cases/~hmr-resilience/expected/check.js @@ -0,0 +1,19 @@ +(window["webpackJsonp"] = window["webpackJsonp"] || []).push([["check"],{ + +/***/ "./index.css": +/***/ (function(module, exports, __webpack_require__) { + +// extracted by mini-css-extract-plugin + if(true) { + // 1479427200000 + var cssReload = __webpack_require__("../../../src/hmr/hotModuleReplacement.js")(module.i, {"hmr":true,"locals":false}); + module.hot.dispose(cssReload); + module.hot.accept(function(){}); + module.hot.accept(undefined, cssReload); + } + +throw new Error("ModuleBuildError: Module build failed (from ./error-loader.js):\nError: I am error\n at Object.loader (D:\\github_git\\mini-css-extract-plugin\\test\\cases\\~hmr-resilience\\error-loader.js:3:12)"); + +/***/ }) + +}]); \ No newline at end of file diff --git a/test/cases/~hmr-resilience/index.css b/test/cases/~hmr-resilience/index.css new file mode 100644 index 00000000..9cad053c --- /dev/null +++ b/test/cases/~hmr-resilience/index.css @@ -0,0 +1,3 @@ +.a { + background: red; +} diff --git a/test/cases/~hmr-resilience/index.js b/test/cases/~hmr-resilience/index.js new file mode 100644 index 00000000..6a9a4b13 --- /dev/null +++ b/test/cases/~hmr-resilience/index.js @@ -0,0 +1 @@ +import './index.css'; diff --git a/test/cases/~hmr-resilience/webpack.config.js b/test/cases/~hmr-resilience/webpack.config.js new file mode 100644 index 00000000..818d58b6 --- /dev/null +++ b/test/cases/~hmr-resilience/webpack.config.js @@ -0,0 +1,62 @@ +import { HotModuleReplacementPlugin } from 'webpack'; + +import Self from '../../../src'; + +module.exports = { + entry: './index.css', + mode: 'development', + devtool: false, + output: { + filename: '[name].js', + }, + optimization: { + runtimeChunk: 'single', + namedModules: true, + namedChunks: true, + splitChunks: { + chunks: 'all', + cacheGroups: { + vendors: { + test: /[\\/]index\.css$/, + name: 'check', + enforce: true, + }, + }, + }, + }, + module: { + rules: [ + { + test: /\.css$/, + use: [ + { + loader: Self.loader, + options: { + hmr: true, + }, + }, + require.resolve('./error-loader'), + ], + }, + ], + }, + plugins: [ + new HotModuleReplacementPlugin(), + new Self({ + filename: '[name].css', + }), + { + apply(compiler) { + compiler.hooks.emit.tapAsync('no-emit', (compilation, callback) => { + const { assets } = compilation; + + // Not interested in comparing output for these. + delete assets['runtime.js']; + delete assets['main.js']; + + callback(); + }); + }, + }, + ], +}; From ad42401510876fc71f1960e4971a07386266328d Mon Sep 17 00:00:00 2001 From: rjgotten Date: Wed, 24 Jul 2019 11:00:45 +0200 Subject: [PATCH 3/5] fix: amends hmr error recovery tests Needed to erase stack trace location, which varies by system --- test/TestCases.test.js | 28 ++++++++++++++++++-- test/cases/~hmr-resilience/expected/check.js | 2 +- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/test/TestCases.test.js b/test/TestCases.test.js index 2f04fe57..c5a3da09 100644 --- a/test/TestCases.test.js +++ b/test/TestCases.test.js @@ -29,16 +29,40 @@ describe('TestCases', () => { const casesDirectory = path.resolve(__dirname, 'cases'); const outputDirectory = path.resolve(__dirname, 'js'); - // HMR tests use and render Date.now + // The ~hmr-resilience testcase has a few variable components in its + // output. To resolve these and make the output predictible and comparable: + // - it needs Date.now to be mocked to a constant value. + // - it needs JSON.stringify to be mocked to strip source location path + // out of a stringified error. let dateNowMock = null; + let jsonStringifyMock = null; beforeEach(() => { dateNowMock = jest .spyOn(Date, 'now') .mockImplementation(() => 1479427200000); + + const stringify = JSON.stringify.bind(JSON); + jsonStringifyMock = jest + .spyOn(JSON, 'stringify') + .mockImplementation((value) => { + // ~hmr-resilience testcase. Need to erase stack trace location, + // which varies by system and cannot be compared. + if (typeof value === 'string' && value.includes('error-loader.js')) { + return stringify( + value.replace( + /\([^(]+error-loader\.js:\d+:\d+\)$/, + '(error-loader.js:1:1)' + ) + ); + } + + return stringify(value); + }); }); afterEach(() => { - dateNowMock.mockClear(); + dateNowMock.mockRestore(); + jsonStringifyMock.mockRestore(); }); for (const directory of fs.readdirSync(casesDirectory)) { diff --git a/test/cases/~hmr-resilience/expected/check.js b/test/cases/~hmr-resilience/expected/check.js index b4b6d8e5..a7261a59 100644 --- a/test/cases/~hmr-resilience/expected/check.js +++ b/test/cases/~hmr-resilience/expected/check.js @@ -12,7 +12,7 @@ module.hot.accept(undefined, cssReload); } -throw new Error("ModuleBuildError: Module build failed (from ./error-loader.js):\nError: I am error\n at Object.loader (D:\\github_git\\mini-css-extract-plugin\\test\\cases\\~hmr-resilience\\error-loader.js:3:12)"); +throw new Error("ModuleBuildError: Module build failed (from ./error-loader.js):\nError: I am error\n at Object.loader (error-loader.js:1:1)"); /***/ }) From ed9f29570d40b514ab759abdb86393cb030e6564 Mon Sep 17 00:00:00 2001 From: rjgotten Date: Wed, 24 Jul 2019 11:25:27 +0200 Subject: [PATCH 4/5] fix: remove unused testcase file --- test/cases/~hmr-resilience/index.js | 1 - 1 file changed, 1 deletion(-) delete mode 100644 test/cases/~hmr-resilience/index.js diff --git a/test/cases/~hmr-resilience/index.js b/test/cases/~hmr-resilience/index.js deleted file mode 100644 index 6a9a4b13..00000000 --- a/test/cases/~hmr-resilience/index.js +++ /dev/null @@ -1 +0,0 @@ -import './index.css'; From 380f8228655d5b854287646636e8c127d73ed041 Mon Sep 17 00:00:00 2001 From: rjgotten Date: Wed, 24 Jul 2019 15:51:33 +0200 Subject: [PATCH 5/5] fix: hmr error recovery review remarks - removes obsolete output filename from testcase; - renames testcase to remove ~ from directory name; - adds comment explaining use of optimization chunks in testcase; - adds comment explaining self-accept with empty error handler --- src/loader.js | 33 ++++++++++++------- test/TestCases.test.js | 2 +- .../error-loader.js | 0 .../expected/check.js | 0 .../index.css | 0 .../webpack.config.js | 8 +++-- 6 files changed, 28 insertions(+), 15 deletions(-) rename test/cases/{~hmr-resilience => hmr-resilience-fail}/error-loader.js (100%) rename test/cases/{~hmr-resilience => hmr-resilience-fail}/expected/check.js (100%) rename test/cases/{~hmr-resilience => hmr-resilience-fail}/index.css (100%) rename test/cases/{~hmr-resilience => hmr-resilience-fail}/webpack.config.js (86%) diff --git a/src/loader.js b/src/loader.js index 6c549210..cbe0ce7e 100644 --- a/src/loader.js +++ b/src/loader.js @@ -16,22 +16,33 @@ const MODULE_TYPE = 'css/mini-extract'; const pluginName = 'mini-css-extract-plugin'; function hotLoader(content, context) { - const accept = context.locals - ? '' - : 'module.hot.accept(undefined, cssReload);'; + const cssReload = loaderUtils.stringifyRequest( + context.context, + path.join(__dirname, 'hmr/hotModuleReplacement.js') + ); + + const cssReloadArgs = JSON.stringify({ + ...context.options, + locals: !!context.locals, + }); + + // The module should *always* self-accept and have an error handler + // present to ensure a faulting module does not bubble further out. + // The error handler itself does not actually need to do anything. + // + // When there are no locals, then the module should also accept + // changes on an empty set of dependencies and execute the css + // reloader. + let accept = 'module.hot.accept(function(){});'; + if (!context.locals) { + accept += '\n module.hot.accept(undefined, cssReload);'; + } return `${content} if(module.hot) { // ${Date.now()} - var cssReload = require(${loaderUtils.stringifyRequest( - context.context, - path.join(__dirname, 'hmr/hotModuleReplacement.js') - )})(module.id, ${JSON.stringify({ - ...context.options, - locals: !!context.locals, - })}); + var cssReload = require(${cssReload})(module.id, ${cssReloadArgs}); module.hot.dispose(cssReload); - module.hot.accept(function(){}); ${accept} } `; diff --git a/test/TestCases.test.js b/test/TestCases.test.js index c5a3da09..6d1394de 100644 --- a/test/TestCases.test.js +++ b/test/TestCases.test.js @@ -67,7 +67,7 @@ describe('TestCases', () => { for (const directory of fs.readdirSync(casesDirectory)) { if (!/^(\.|_)/.test(directory)) { - const expectsError = /^~/.test(directory); + const expectsError = /-fail$/.test(directory); // eslint-disable-next-line no-loop-func it(`${directory} should compile to the expected result`, (done) => { diff --git a/test/cases/~hmr-resilience/error-loader.js b/test/cases/hmr-resilience-fail/error-loader.js similarity index 100% rename from test/cases/~hmr-resilience/error-loader.js rename to test/cases/hmr-resilience-fail/error-loader.js diff --git a/test/cases/~hmr-resilience/expected/check.js b/test/cases/hmr-resilience-fail/expected/check.js similarity index 100% rename from test/cases/~hmr-resilience/expected/check.js rename to test/cases/hmr-resilience-fail/expected/check.js diff --git a/test/cases/~hmr-resilience/index.css b/test/cases/hmr-resilience-fail/index.css similarity index 100% rename from test/cases/~hmr-resilience/index.css rename to test/cases/hmr-resilience-fail/index.css diff --git a/test/cases/~hmr-resilience/webpack.config.js b/test/cases/hmr-resilience-fail/webpack.config.js similarity index 86% rename from test/cases/~hmr-resilience/webpack.config.js rename to test/cases/hmr-resilience-fail/webpack.config.js index 818d58b6..08d44853 100644 --- a/test/cases/~hmr-resilience/webpack.config.js +++ b/test/cases/hmr-resilience-fail/webpack.config.js @@ -6,9 +6,11 @@ module.exports = { entry: './index.css', mode: 'development', devtool: false, - output: { - filename: '[name].js', - }, + // NOTE: + // Using optimization settings to shunt everything + // except the generated module code itself into + // discarded chunks that won't be compared for + // expected output. optimization: { runtimeChunk: 'single', namedModules: true,