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
Add react-refresh for WDS #5840
Add react-refresh for WDS #5840
Conversation
frontend/package.json
Outdated
"peerDependencies": { | ||
"type-fest": "0.16.0" | ||
}, |
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.
Why is this peer dependency here?
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.
from https://github.com/pmmmwh/react-refresh-webpack-plugin/blob/main/README.md
Note 1: If you use webpack.config.ts, please also install type-fest as a peer dependency.
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.
But peer dependencies aren't installed by default.
If this is required then it should be a dev dependency. But if everything works without it, then just this peer dependency.
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.
well everything works like this, also he mentions in the readme to install is as peer.
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.
as @christianvogt said, the peer dependencies are not installed. It seems to me that the README is just worded weirdly - type-fest
is peerDepedency
of react-refresh-webpack-plugin
which means it should be in our devDependencies
6beb4f9
to
1389074
Compare
Getting this warning when the page loads for the first time, it doesn't seem to hurt functionality, and it doesn't appear again. |
While I don't know the side effect of this error, hot reloading worked for me in testing. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christianvogt, glekner The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
Ok, lets try this again.
This is the same code from the previous PR with 2 changes:
react-refresh
will only trigger on WDS scenarios, omittingwebpack --mode=development
.This change fixes
yarn check-cycles
andyarn dev-once
transportMode
tows
instead ofSockJS
:ws
mode will become the default mode in the next major devServer version. This mode uses ws as a server, and native WebSockets on the client.This fixes the
iframe
errors, Topology view now works on my env.ws
is now the recommended approach, and 97.57%of browsers support it
@christianvogt @rawagner @spadgett
can we review this again and make sure it all works? this feature is a blessing :)