Skip to content

Conversation

@zouhir
Copy link
Member

@zouhir zouhir commented Oct 21, 2017

I have a little preact-cli project I work with on weekends.

Today HMR stopped working and I am able to get it to work back via this fix.

dev-server was not emitting socket-js with the dev bundle and seems like the combo of Webpack HMR plugin + hot:true in dev-server options is not working.

This fix will ensure in Dev mode, the required socket stuff are committed to hmr bundle and the reload happens.

is there better ideas? happy to discuss.

Copy link
Member

@reznord reznord left a comment

Choose a reason for hiding this comment

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

Thanks a ton @zouhir ❤️ 😍

};

if (!isProd) {
entry['hmr'] = `webpack-dev-server/client?http://localhost:${process.env.PORT || env.port || 8080}`;
Copy link
Member

Choose a reason for hiding this comment

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

This is a temporary fix with with we can go for now. But I guess the HMR failure is related to webpack-dev-server or html-webpack-plugin.

Once it is fixed in the upstream, we can clean this off.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's webpack-dev-server.

Copy link
Member

@developit developit left a comment

Choose a reason for hiding this comment

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

Any chance the port change is unnecessary here?

@reznord reznord merged commit 047e922 into preactjs:master Oct 21, 2017
@reznord reznord mentioned this pull request Oct 21, 2017
@zouhir
Copy link
Member Author

zouhir commented Oct 21, 2017

@developit the port in here has to exactly be your devServer port option otherwise you'll get console messages that socket can't connect, therefore no hmr.

Copy link
Member

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

Nice, thanks @zouhir! Totally forgot about this approach 🎉

@marvinhagemeister
Copy link
Member

Thank you so much for fixing this! We definitely tort this upstream in case someone hasn't already.

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.

5 participants