Skip to content

feat: hmr error recovery #435

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

feat: hmr error recovery #435

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
17acb10
feat: hmr error recovery
rjgotten Jul 22, 2019
607532d
feat: hmr error recovery test
rjgotten Jul 24, 2019
ad42401
fix: amends hmr error recovery tests
rjgotten Jul 24, 2019
ed9f295
fix: remove unused testcase file
rjgotten Jul 24, 2019
380f822
fix: hmr error recovery review remarks
rjgotten Jul 24, 2019
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
View file
Open in desktop
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 •
edited
Loading

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