-
-
Notifications
You must be signed in to change notification settings - Fork 199
Adding Hot Module Replacement support #8
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
Conversation
Hey @alOneh! Thanks for getting this going :). One of the reasons this hadn't been added yet was that, afaik, even if you enable HMR like this... nothing will actually take advantage of it. I mean, CSS doesn't work with HMR (because we're using the ExtractTextWebpackPlugin), the React HMR stuff isn't configured yet, etc. When we enable HMR, I want it to work with at least one thing behind the scenes. Or (after this PR) does it already work with something and I didn't realize? Thanks! |
lib/config-generator.js
Outdated
@@ -463,6 +467,7 @@ class ConfigGenerator { | |||
// avoid CORS concerns trying to load things like fonts from the dev server | |||
headers: { 'Access-Control-Allow-Origin': '*' }, | |||
// required by FriendlyErrorsWebpackPlugin | |||
hot: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about requiring the user to pass the --hot
flag in at the command line:
./node_modules/.bin/encore dev-server --hot
Then, we would detect this (similar to how we're intercepting a few options already in the encore
executable, like --https
) and use it above to conditionally add HotModuleReplacementPlugin
. Otherwise, we're forcing the dev-server
to only be used with hot
mode (I'm not sure if there's an issue with that, but forcing it feels a little weird)
This patch work with the classic case, getting your browser reloading after updating any css. As the guide mention, we don't need a lot of config to get this working. For React HMR we have to do it differently. |
ok, I get your point ! I find some answer in extract-text-webpack-plugin issues, we can give a try to css-hot-loader to fix our use case. |
@@ -413,6 +413,10 @@ class ConfigGenerator { | |||
const outputPath = this.webpackConfig.outputPath.replace(this.webpackConfig.getContext() + '/', ''); | |||
plugins.push(new AssetsOutputDisplayPlugin(outputPath, friendlyErrorsPlugin)); | |||
|
|||
if (this.webpackConfig.useDevServer()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may not make any difference, but should we also use this.webpackConfig.useHotModuleReplacementPlugin()
? for this if
statement as well?
Very nice updates to the PR! We could definitely merge this without adding any explicit support for anything (yet). I think the PR is basically ready. About |
And we may also have this same issue (especially with |
Merged - but see #16 for a small fix. And also, we still need to add explicit support for HMR (i.e. in React... and if possible, with CSS) |
For css HMR when using ExtractTextWebpackPlugin
|
This PR is a patch to provide simply add HMR support if the webpack-dev-server is use.
Sorry I don't know if there is a standard for PR on this repo.
To test, run
.node_modules/.bin/encore dev-server
and refresh the page of your project, then open the console and you should see[WDS] Hot Module Replacement enabled.
I think I can find a way to test it with the test suite.