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: false doesn't disable Hot Module Replacement #392

Merged
merged 1 commit into from
Dec 29, 2023

Conversation

thedanbob
Copy link
Contributor

@thedanbob thedanbob commented Dec 22, 2023

Summary

webpacker-dev-server >= 4 enables HMR by default, so explicitly disable when hmr: false. Fixes #390

Pull Request checklist

  • Add/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file

@thedanbob thedanbob force-pushed the hmr-fix branch 2 times, most recently from bb8ea3a to ec1d7a2 Compare December 22, 2023 14:50
@thedanbob thedanbob changed the title fix: hmr: false doesn't disable Hot Module Reloading fix: hmr: false doesn't disable Hot Module Replacement Dec 22, 2023
Copy link
Contributor

@ahangarha ahangarha left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I just made some suggestions. What do you think about them?

lib/shakapacker/dev_server_runner.rb Outdated Show resolved Hide resolved
@justin808
Copy link
Member

@thedanbob can I get a link to the docs or source code of the '--no-hot' option?

@ahangarha
Copy link
Contributor

@justin808
In the readme of the webpack-dev-server, it is mentioned.

Apart from that, all of the options in webpack CLI can be reverted by using no prefix. This is why no explicit reference or implementation of --no-hot exists in the code base. --no-hot is simply the opposite of --hot, so it sets hot to false.

@thedanbob
Copy link
Contributor Author

@justin808 I just checked and this would work as well, and is perhaps cleaner:

cmd += ["--hot", (@hot || false).to_s] if @hot != true

Of course I'll have to adjust the tests to match. Probably making the default (in the tests) as hmr: true would be best, as @ahangarha suggested.

@ahangarha
Copy link
Contributor

@thedanbob

cmd += ["--hot", (@hot || false).to_s] if @hot != true

I prefer a more visible code in more lines over one-liners. It is simpler to read, understand, and debug.

@thedanbob thedanbob force-pushed the hmr-fix branch 2 times, most recently from 73d7195 to acf1709 Compare December 24, 2023 16:42
@thedanbob
Copy link
Contributor Author

thedanbob commented Dec 24, 2023

How does this look? I went ahead and changed the tests to use hmr: true. My implementation did have a side effect on a couple of inlining_css? tests but I think the behavior change is unimportant? Edit: fixed this.

Copy link
Contributor

@ahangarha ahangarha left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. I Shared my concerns in the comments.

lib/shakapacker/dev_server_runner.rb Outdated Show resolved Hide resolved
lib/shakapacker/dev_server_runner.rb Outdated Show resolved Hide resolved
* webpacker-dev-server >= 4 enables HMR by default, so explicitly disable when `hmr: false`
Copy link
Contributor

@ahangarha ahangarha left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Thanks.

@justin808 justin808 merged commit 0c3fb7e into shakacode:master Dec 29, 2023
41 checks passed
@justin808
Copy link
Member

Thanks @thedanbob!

@thedanbob thedanbob deleted the hmr-fix branch January 1, 2024 20:21
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.

hmr: false does not disable Hot Module Replacement
3 participants