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: check for name of HotModuleReplacementPlugin to avoid RangeError #2146

Merged
merged 3 commits into from Jul 27, 2019

Conversation

jdb8
Copy link
Contributor

@jdb8 jdb8 commented Jul 23, 2019

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

Fixes #87, a long-standing subtle bug which seems to have tripped up a lot of people historically.

Breaking Changes

No breaking changes.

Additional Info

See my comment on the issue: #87 (comment). The only way I can see this solution being a problem is if we're worried about configs that are using --hot and also have a plugin that isn't HotModuleReplacementPlugin but has the same name. In that case, we'd no longer inject the real HMR plugin. I'm not sure how likely this is and I seem to remember seeing other webpack plugins that check the name string rather than relying on the constructor reference (probably for this same reason).

However, if we are concerned and if there's a property we can additionally check on the instances of the plugin, that would potentially be more robust. Ideally it'd be a property that's existing on the HotModuleReplacementPlugin for a while to make it as backward-compatible as possible.

@codecov
Copy link

codecov bot commented Jul 23, 2019

Codecov Report

Merging #2146 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2146   +/-   ##
=======================================
  Coverage   92.56%   92.56%           
=======================================
  Files          33       33           
  Lines        1265     1265           
  Branches      361      361           
=======================================
  Hits         1171     1171           
  Misses         87       87           
  Partials        7        7
Impacted Files Coverage Δ
lib/utils/addEntries.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6653e7...8537ad6. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Good job, thanks!

@alexander-akait alexander-akait merged commit 4579775 into webpack:master Jul 27, 2019
@jdb8 jdb8 deleted the check-hmr-plugin-name branch July 29, 2019 18:50
knagaitsev pushed a commit to knagaitsev/webpack-dev-server that referenced this pull request Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

~~Anyone else receiving an error Uncaught RangeError: Maximum call stack size exceeded after one hot reload?~~
5 participants