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

Fix HMR failure with js error on load, take 2 #2531

Merged
merged 8 commits into from
Mar 6, 2019
Merged

Conversation

mischnic
Copy link
Member

@mischnic mischnic commented Jan 11, 2019

↪️ Pull Request

See example.

I'm not sure if my changes have some side effect?
Might have a negative effect on scope-hoisting build sizes.

Fixes #879, previous attempt: #898

💻 Examples

On first load:

throw new Error("STOP");

console.log("test");

Removing the Error line doesn't hmr successfully.

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@@ -103,5 +111,12 @@ parcelRequire = (function (modules, cache, entry, globalName) {
}

// Override the current require with this new one
parcelRequire = newRequire;
Copy link
Member

Choose a reason for hiding this comment

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

This seems dangerous. Why are we overwriting the global variable here? Won't that already happen when the function returns?

Copy link
Member Author

@mischnic mischnic Mar 6, 2019

Choose a reason for hiding this comment

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

Because returning doesn't work if an error is thrown and parcelRequire is then undefined for subsequent HMR updates

parcelRequire = newRequire;
if(error){
// throw error from earlier, _after updating parcelRequire_
throw error;
}
return newRequire;

And this overwrites a global variable as well:

parcelRequire = (function (modules, cache, entry, globalName) {

@devongovett devongovett merged commit 30624f7 into master Mar 6, 2019
@devongovett devongovett deleted the hmr_error_on_start branch March 6, 2019 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants