fix(gatsby): show @hot-loader installation prompt #26927
Conversation
gatsbot
bot
added
the
status: triage needed
wardpeet
added
status: needs core review
There was a problem hiding this comment.
Hey Ward! Took a look at the docs part of this, and left a few suggestions.
One broader suggestion: it might help clarify things to split out the example stuff (lines 272-280) under its own header. That way, people don't accidentally just copy the npm install line and all end up with the same version. Let me know what you think!
meganesu
added
type: documentation
Co-authored-by: Megan Sullivan <[email protected]>
Co-authored-by: Megan Sullivan <[email protected]>
wardpeet
added
the
bot: merge on green
| const { version: reactDomVersion } = require(`react-dom/package.json`) | ||
| const { major, minor } = semver.parse(reactDomVersion) | ||
| console.warn( | ||
| `React-Hot-Loader: please install "@hot-loader/react-dom@${major}.${minor}" to make sure all features of React are working.` |
There was a problem hiding this comment.
patch part doesn't need to match?
There was a problem hiding this comment.
it's not available for every patch version of react so that's the reason why I didn't do it.
There was a problem hiding this comment.
What will happen when there is version mismatch between react and @hot-loader/react-dom (given that patch is not available for everything, I guess we could limit question to different minor versions)?
I guess the behaviour itself is undefined - but would users see any warnings about this (that's going from the messaging here that major and minor should be the same for those 2 packages)?
There was a problem hiding this comment.
I've added an extra warning to the code, in my tests if your version is higher I haven't found any immediate issue but of course, there are probably some subtle changes.
| resolve.alias[`react-dom/server$`] = require.resolve(`react-dom/server`) | ||
| resolve.alias[`react-dom`] = require.resolve(`@hot-loader/react-dom`) | ||
| } catch (err) { | ||
| const { version: reactDomVersion } = require(`react-dom/package.json`) |
There was a problem hiding this comment.
@hot-loader/react-dom has versions starting from 16.7.0. Technically we support ^16.4.2 so the warning is not actionable for users on react versions between those 2 - worth silencing the warning if version is below 16.7? There are no hooks there anyway (assuming this is only needed for hooks support)
There was a problem hiding this comment.
I'll do 16.8.0 because that's when hooks are a thing and you'll only need it for hooks.
There was a problem hiding this comment.
I don't know specific details but React 16.6+ features may not work. from the error/warning message implies it's not just hooks? (or maybe message is just not updated - not sure if I remember correctly but hooks were expected to land in 16.7 but didn't)
There was a problem hiding this comment.
Improved semver checks. These are all the patches that they have needed to do https://github.com/gaearon/react-hot-loader/blob/master/src/webpack/patch.js
Description
We have a long-standing issue that complains about an HMR warning #11934. It can be resolved by adding
@hot-loader/react-domas an alias to react-dom. It's hard to know that you have to do it.This PR adds a warning to the cli output so people can install it. We auto-add the webpack aliases. I've also added a section to common errors to clarify this a little bit better.
Documentation
I've added a section to common errors to point people towards.
Related Issues
Fixes #11934