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

Additional follow-up fix for #1704 to remove remaining warnings #1736

Merged

Conversation

heath-freenome
Copy link
Contributor

  • 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

…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
@vercel
Copy link

vercel bot commented Oct 20, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/jared/razzle/GE7BiptQE6VwDvkNDE7VcFG6CRdv
✅ Preview: https://razzle-git-fork-heath-freenome-additional-update-t-083719-jared.vercel.app

@heath-freenome
Copy link
Contributor Author

@fivethreeo I believe I fixed everything although I wasn't fully able to verify the webpackHotDevClient change

@heath-freenome
Copy link
Contributor Author

@fivethreeo ok, I'm done fixing the little things... Looking forward to your review

@heath-freenome
Copy link
Contributor Author

heath-freenome commented Oct 21, 2021

@fivethreeo Actually, I think this PR (specifically the start.js change) will fix the issue I saw with my App not working with code splitting... Here's why I think it is so:

Using listen() with v4 I get the createSocketURL warning:

WARNING in (webpack)-dev-server/client/utils/createSocketURL.js
There are multiple modules with names that only differ in casing.
This can lead to unexpected behavior when compiling on a filesystem with other case-semantic.
Use equal casing. Compare these module identifiers:
* /Users/heath/dev/razzle/node_modules/webpack-dev-server/client/utils/createSocketURL.js
    Used by 2 module(s), i. e.
    /Users/heath/dev/razzle/node_modules/webpack-dev-server/client/index.js?protocol=ws%3A&hostname=err+%3D%3E+%7B%0A++++++++++++++++if+%28err%29+%7B%0A++++++++++++++++++logger.error%28err%29%3B%0A++++++++++++++++%7D%0A++++++++++++++%7D&port=3001&pathname=%2Fws&logging=none
* /Users/heath/dev/razzle/node_modules/webpack-dev-server/client/utils/createSocketUrl.js
    Used by 1 module(s), i. e.
    /Users/heath/dev/razzle/packages/razzle-dev-utils/webpackHotDevClient.js
 @ (webpack)-dev-server/client/utils/createSocketURL.js
 @ (webpack)-dev-server/client?protocol=ws%3A&hostname=err+%3D%3E+%7B%0A++++++++++++++++if+%28err%29+%7B%0A++++++++++++++++++logger.error%28err%29%3B%0A++++++++++++++++%7D%0A++++++++++++++%7D&port=3001&pathname=%2Fws&logging=none
 @ multi (webpack)-dev-server/client?protocol=ws%3A&hostname=err+%3D%3E+%7B%0A++++++++++++++++if+%28err%29+%7B%0A++++++++++++++++++logger.error%28err%29%3B%0A++++++++++++++++%7D%0A++++++++++++++%7D&port=3001&pathname=%2Fws&logging=none (webpack)/hot/dev-server.js /Users/heath/dev/razzle/packages/razzle-dev-utils/webpackHotDevClient.js ./src/client

If you look closely you can see that hostname is set to some crazy value: hostname=err+%3D%3E+%7B%0A++++++++++++++++if+%28err%29+%7B%0A++++++++++++++++++logger.error%28err%29%3B%0A++++++++++++++++%7D%0A++++++++++++++%7D&

Using start() I see:

WARNING in (webpack)-dev-server/client/utils/createSocketURL.js
There are multiple modules with names that only differ in casing.
This can lead to unexpected behavior when compiling on a filesystem with other case-semantic.
Use equal casing. Compare these module identifiers:
* /Users/heath/dev/razzle/node_modules/webpack-dev-server/client/utils/createSocketURL.js
    Used by 3 module(s), i. e.
    /Users/heath/dev/razzle/node_modules/webpack-dev-server/client/index.js?protocol=ws%3A&hostname=localhost&port=3001&pathname=%2Fws&logging=none
* /Users/heath/dev/razzle/node_modules/webpack-dev-server/client/utils/createSocketUrl.js
    Used by 1 module(s), i. e.
    /Users/heath/dev/razzle/packages/razzle-dev-utils/webpackHotDevClient.js
 @ (webpack)-dev-server/client/utils/createSocketURL.js
 @ (webpack)-dev-server/client?protocol=ws%3A&hostname=localhost&port=3001&pathname=%2Fws&logging=none
 @ multi (webpack)-dev-server/client?protocol=ws%3A&hostname=localhost&port=3001&pathname=%2Fws&logging=none (webpack)/hot/dev-server.js /Users/heath/dev/razzle/packages/razzle-dev-utils/webpackHotDevClient.js ./src/client

As you can see the hostname is properly assigned to localhost... So oddly, that warning helped me fix the breaking issue I was experiencing...

And hopefully the changes I made in webpackHotDevClient.js eliminates the above warning. My local builds still seem to pick up a different version of that client...

@@ -850,6 +847,7 @@ module.exports = (
// See https://github.com/facebookincubator/create-react-app/issues/387.
disableDotRule: true,
},
hot: true,

Choose a reason for hiding this comment

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

👍

});
if (devServerMajorVersion > 3) {
// listen was deprecated in v4 and causes issues when used, switch to its replacement
clientDevServer.start();

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);

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.

Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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
@chrisdostert
Copy link

@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

@chrisdostert
Copy link

@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

@heath-freenome
Copy link
Contributor Author

@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

@chrisdostert @fivethreeo
#1740

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants