Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: hmr error recovery #435

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions src/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand All @@ -152,6 +166,10 @@ export function pitch(request) {
this.addContextDependency(dep);
}, this);

if (compilation.errors.length > 0) {
return callback(compilation.errors[0]);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For catching errors, right?

Copy link
Author

@rjgotten rjgotten Jul 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It was moved down passed the dependencies, to ensure those dependenices are always tracked.

Flipping the error state into a success state on the parent compilation, means that it will also update its registered dependencies. (If the parent compilation would fail, it would retain the previous set.)

If we'd drop out of the child compilation because of an error before the dependencies of the child compilation are moved into the parent, then we would 'lose' those dependencies, including any change watches on them. This way, we won't.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there be a case where multiple errors need to be shown in the errors array?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ScriptedAlchemy
Not sure actually. The current version of the plugin only reports the first child compilation error as well.


if (!source) {
return callback(new Error("Didn't get a result from child compiler"));
}
Expand Down
42 changes: 40 additions & 2 deletions test/TestCases.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,46 @@ describe('TestCases', () => {
const casesDirectory = path.resolve(__dirname, 'cases');
const outputDirectory = path.resolve(__dirname, 'js');

// 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.mockRestore();
jsonStringifyMock.mockRestore();
});

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);
Expand Down Expand Up @@ -59,7 +97,7 @@ describe('TestCases', () => {
}

webpack(webpackConfig, (err, stats) => {
if (err) {
if (err && !expectsError) {
done(err);
return;
}
Expand All @@ -76,7 +114,7 @@ describe('TestCases', () => {
})
);

if (stats.hasErrors()) {
if (stats.hasErrors() && !expectsError) {
done(
new Error(
stats.toString({
Expand Down
4 changes: 4 additions & 0 deletions test/cases/~hmr-resilience/error-loader.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
module.exports = function loader() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use ~ in filename

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any other suggestion then for a moniker to use to indicate tests that are expected to fail?

Testcases.test.js works by enumerating directory names and running all the cases, then bailing if one of them has a compilation error. But here we have a testcase which is expected to have a compilation error. First of its kind.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rjgotten just use fail word in test name

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted the test pattern in TestCases.test.js to /-fail$/ i.e. any test suffixed with -fail is expected to error. And in accordance renamed the testcase to hmr-resilience-fail, getting rid of the ~.

const callback = this.async();
callback(new Error('I am error'));
};
19 changes: 19 additions & 0 deletions test/cases/~hmr-resilience/expected/check.js
Original file line number Diff line number Diff line change
@@ -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(){});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little more context please:
why what ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need this module.hot.accept(function(){});? here, please add comment to source doe, because this line is misleading for other developers

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Will add a small description in the loader.js where this particular line of code is emitted.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

module.hot.accept(undefined, cssReload);
}

throw new Error("ModuleBuildError: Module build failed (from ./error-loader.js):\nError: I am error\n at Object.loader (error-loader.js:1:1)");

/***/ })

}]);
3 changes: 3 additions & 0 deletions test/cases/~hmr-resilience/index.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.a {
background: red;
}
62 changes: 62 additions & 0 deletions test/cases/~hmr-resilience/webpack.config.js
Original file line number Diff line number Diff line change
@@ -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: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it is necessary for test?

Copy link
Author

@rjgotten rjgotten Jul 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

output.filename is a leftover. I'll remove it.

The optimization settings are there to force everything but the compiled module itself into a separate chunk (which is then also not emitted).
This limits the surface of the test, so we don't end up comparing code for e.g. the hot reloader client itself. (Which may actually be subject to change and cause false-negative failed unit tests if it is ever updated.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the filename leftover, and left a comment explaining why the optimization chunk settings are being used.

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();
});
},
},
],
};