Unhandled HMR updates should cause a page reload

[mischnic]

↪️ Pull Request

As mentioned in #289 (comment)

Closes #289, closes #2675, closes #2606, closes #2455, closes #2509

💻 Examples

Would case a full reload:

document.body.innerHTML += "Test21213asdadsd<br>";

Would not:

document.body.innerHTML += "Test21213asdadsd<br>";

if(module.hot){
	module.hot.accept(()=>{
		console.log("Hello");
	})
}

🚨 Test instructions

See example

✔️ 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

[mischnic]

I just noticed a major flaw: this breaks the "React HMR just works" feature, because re-executing all Javascript is fine with React. But it doesn't work with setInterval (#2606) or HTML custom elements (#2675).

Possible solutions:

• disabling this change when react is used (but would also be needed for preact, ...)
• only adding the reload flag (Add --reload flag #2677)

[DeMoorJasper]

If this breaks react it will likely break a lot more libraries/frameworks.

I think the reload flag would probably be our best bet in this case

[mischnic]

Closing as this change would break the other half of libraries (current React et al. work and setTimeout etc. don't).

[mischnic]

A downside: everything runs twice, before the reload and after the reload. Could still cause issues in some infinite loop situations.

[devongovett]

mmm, so really what we should do is do two passes: 1 to check if any module will accept, and another to actually re-execute the modules if so, or reload the page if not.

[mischnic]

to check if any module will accept

= does any asset have a accept or dispose callback. I think that's correct

[devongovett]

This isn't quite enough. You need to traverse upward from the asset to the root asset and check if any of the parents accept the update as well as the asset itself. Basically, we need to split hmrAccept up into two functions. One that traverses upward and checks if an update should be accepted, but without actually re-executing them. It can also keep track of all of the assets that need to be re-executed in a list. Then, if an update is accepted, go through the list we tracked earlier, and re-execute them.

[mischnic]

Done: extracted the actual execution into hmrAcceptRun and kept track of them in assetsToAccept

[devongovett]

🎉

[louh]

Hey, I wanted to comment here because when we updated our parcel-bundler to 1.12.3 we found that HMR is broken. (issue: streetmix/streetmix#1357 (comment)) Only by reverting to an earlier version we would get it back, but that's what let me down this rabbit hole to try and find what might have broken in 1.12.x.

When we adopted Parcel initially, we did have issues with the HMR (as many people have) but we gradually resolved each of our issues and now HMR works quite well with our app. As a result, it was upsetting to discover that default HMR functionality completely broke. Usually, when an API changes in a library, I expect to see a large, bold "BREAKING CHANGE" notice in the Changelog, which would have given us context and an ability to properly handle the change. Ideally, this would have also been a major version bump as this functionality was introduced without being backwards-compatible. (I think, as a minor version bump, the ideal solution would have been to disable HMR with a flag, instead of changing the default.)

I now know what to do, but I think the fix shouldn't have been hidden in a comment in another issue (#289 (comment)).

I'll be looking into making a pull request to update your documentation and Changelog for people who might be running into similar issues. Thanks for your work on Parcel!

[wyozi]

#2509 still a problem in 2.0.0-alpha.3.2, so not sure this PR fixed that