Remove usages of deprecated React lifecycle methods #3327
Conversation
Hypnosphi
added
addon: info
addon: knobs
ui
maintenance
Hypnosphi
requested review from
alexandrebodin,
alterx,
dangreenisrael,
danielduan,
Gongreg,
igor-dv,
kazupon,
leonrodenburg,
naipath,
ndelangen,
rmevans9,
saponifi3d,
shilman,
theinterned,
thomasbertet and
tmeasday
as code owners
Codecov Report
@@ Coverage Diff @@
## master #3327 +/- ##
=========================================
+ Coverage 35.61% 35.82% +0.2%
=========================================
Files 472 472
Lines 10130 10159 +29
Branches 940 957 +17
=========================================
+ Hits 3608 3639 +31
+ Misses 5936 5916 -20
- Partials 586 604 +18
Continue to review full report at Codecov.
|
| "react": ">=15.0.0 || ^16.0.0", | ||
| "react-dom": ">=15.0.0 || ^16.0.0" | ||
| "react": ">=15.0.0", | ||
| "react-dom": ">=15.0.0" |
There was a problem hiding this comment.
Should this be <18.0.0 as well or something? Do we just not set an upper limit on the version range so we work for as long as possible?
There was a problem hiding this comment.
The whole point of this PR is to make storybook future-compatible. So no, I don't think that we should add new restrictions in it (note that >=15.0.0 part was there before, I just removed the redundant part)
There was a problem hiding this comment.
It’s tricky though, right? I mean sort of by definition react v18 may very well have some incompatibility with what we are doing.
So we just leave it to the user (oh SB breaks with react 18, maybe I should try updating it) rather than the package system?
There was a problem hiding this comment.
Peer dependencies should be as loose as possible. We shouldn't say we're incompatible with something unless we know it for sure. And React tries hard to make major releases breaking only for those projects that had some unfixed deprecation warnings at the moment of release
There was a problem hiding this comment.
Actually you can read the lack of upper limit here as our contract to support next major React releases before they become stable. For 16, it was more than half a year between first alpha and stable release, so I think we can afford such a contract
There was a problem hiding this comment.
contract to support next major React releases before they become stable
Right, but this particular version of Storybook is unlikely[1] to support react@18. No doubt we will try to be on top of it and get a version of SB out well before react@18 is released that does support it, but it won't be this version (storybook@4.0.0 I guess).
[1] Assuming breaking changes in react@18, which of course we don't know, but it is fair to assume.
There was a problem hiding this comment.
So when we do ship the (theoretical) changes that support react@18 in storybook 4.x.y we won't actually change the package.json to indicate that and users won't be able to tell from the output of npm/yarn that they should update to sb@4.x.y if they want to use react@18.
There was a problem hiding this comment.
Anyway, I don't know if it is a big deal as it's not like peer dependencies even really work properly and it all requires users to actually read changelogs/interpret esoteric errors anyway as a result, so I am not saying we have to do it. But it feels more correct.
There was a problem hiding this comment.
Ok I think I got the idea, but I think it's out of scope of this PR. Feel free to open another one, and note that the next major version is 17
| @@ -19,12 +19,19 @@ if (isBrowser) { | |||
| rootEl = document.getElementById('root'); | |||
| } | |||
|
|
|||
| function render(node, el) { | |||
| ReactDOM.render( | |||
| process.env.STORYBOOK_EXAMPLE_APP ? <React.StrictMode>{node}</React.StrictMode> : node, | |||
There was a problem hiding this comment.
Maybe we should add this as a setting so users can also enforce strictness in their storybooks? Either yarn storybook --strict or import { setStrict } from '@storybook/react'; setStrict(true).
I'd probably lean toward the second as it is react only.
There was a problem hiding this comment.
It's quite trivial to add on user side:
addDecorator(story => <StrictMode>{story()}<StrictMode>)
Here I made it on render level just to make sure it captures all our liblrary components (which might not be what end users need)
Issue: #3317
What I did
Added
StrictModeto all our example apps, fixed violations on our side, and opened issues for third-party libraries:https://github.com/issues?utf8=%E2%9C%93&q=is%3Aopen+is%3Aissue+author%3AHypnosphi+archived%3Afalse+lifecycle