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
Additional follow-up fix for #1704 to remove remaining warnings #1736
Additional follow-up fix for #1704 to remove remaining warnings #1736
Conversation
…nings - Added `devServerMajor.js` to `razzle-dev-utils` that refactors the detection of the `webpack-dev-server` version in a manner similar to `webpackMajor.js` - Updated `webpackHotDevClient.js` to use `devServerMajor.js` to pickup the properly cased `createSocketUrl` - Hopefully this approach won't break things like the change that was made previously that ended up getting reverted - Updated `createConfigAsync.js` to use `razzle-dev-utils/devServerMajor` for the `webpack-dev-server` version as well as: - Restored `hot: true` to the common `devServer` config and instead only explicitly adding the `HMR` to the plugins for v3 to avoid the following warning: ``` [webpack-dev-server] "hot: true" automatically applies HMR plugin, you don't have to add it manually to your webpack configuration. ``` - Updated `start.js` script to include `devServerMajor.js` so that v4 will `start/stop` the client dev server rather than `listen/close` used for v3 - Also added `port` into the copied `devServer` config to allow `start()` to work properly
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/jared/razzle/GE7BiptQE6VwDvkNDE7VcFG6CRdv |
@fivethreeo I believe I fixed everything although I wasn't fully able to verify the |
@fivethreeo ok, I'm done fixing the little things... Looking forward to your review |
@fivethreeo Actually, I think this PR (specifically the Using listen() with v4 I get the createSocketURL warning:
If you look closely you can see that Using start() I see:
As you can see the And hopefully the changes I made in |
@@ -850,6 +847,7 @@ module.exports = ( | |||
// See https://github.com/facebookincubator/create-react-app/issues/387. | |||
disableDotRule: true, | |||
}, | |||
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.
👍
packages/razzle/scripts/start.js
Outdated
}); | ||
if (devServerMajorVersion > 3) { | ||
// listen was deprecated in v4 and causes issues when used, switch to its replacement | ||
clientDevServer.start(); |
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.
You could use startCallback if you want to have feature parity on the error handler.
const startCallback = (error) => err && logger.error(err);
clientDevServer.startCallback(startCallback , startCallback);
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.
You can add the same (not parity) to stop with stopCallback, which I believe will only trigger if a middleware is unable to close.
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.
Looking at the source code for startCallback and stopCallback currently, (err) will always be null... But I can add it in case things change in the future. Or maybe I'll just add .catch()
handlers to both start and stop instead... since ultimately we just want to deal with error that are thrown
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.
Good callout.
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.
Ok... Pushed code to support this, assuming that the new issue I filed on webpack-dev-server
gets addressed.
…`start()` and `stop()` with a generic error handler
eafd525
to
8c90455
Compare
@fivethreeo For some reason it seems like devServerMajor.js didn't get included in the package released as 4.2.3. Error: Cannot find module 'razzle-dev-utils/devServerMajor'
Require stack:
- /src/node_modules/razzle/config/createConfigAsync.js
- /src/node_modules/razzle/scripts/start.js |
Ah, I think maybe it needs to be added to files |
|
devServerMajor.js
torazzle-dev-utils
that refactors the detection of thewebpack-dev-server
version in a manner similar towebpackMajor.js
webpackHotDevClient.js
to usedevServerMajor.js
to pickup the properly casedcreateSocketUrl
createConfigAsync.js
to userazzle-dev-utils/devServerMajor
for thewebpack-dev-server
version as well as:hot: true
to the commondevServer
config and instead only explicitly adding theHMR
to the plugins for v3 to avoid the following warning:start.js
script to includedevServerMajor.js
so that v4 willstart/stop
the client dev server rather thanlisten/close
used for v3port
into the copieddevServer
config to allowstart()
to work properly