-
Notifications
You must be signed in to change notification settings - Fork 45.6k
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
Rename inputsAreEqual to areHookInputsEqual & move it to shared #14036
Conversation
ReactDOM: size: 0.0%, gzip: 0.0% Details of bundled changes.Comparing: bf9fadf...cc18195 react-dom
scheduler
Generated by 🚫 dangerJS |
c62663e
to
7f558bc
Compare
7f558bc
to
cc18195
Compare
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.
Hey! Thanks for sending this in!
This looks good from my end, and I like that it drops duplication, particularly for warnings. However someone else more familiar with SSR should take a look at this.
If one code path was missing a warning, is there any concern about that behavior changing?
@sophiebits who do you think is the best person to review this?
Seems OK to me. Shared means that it will get copied into each renderer which I guess is fine since that's effectively what we were manually doing before. Hooks aren't stable so we're allowed to have breaking changes (and we generally feel OK adding warnings in minor releases anyway since they don't affect behavior). |
Thanks! |
I know its bikeshedding but the name of this function triggered my OCD a little bit because it sounds (to me) like a name for a boolean variable. Most commonly other functions with similar purpose are named in the codebase with verb as first work, i.e.
isValidElementType
.While renaming it I've also noticed that this code was duplicated and already out of sync (one having a dev warning, one not having it) so I thought it would be also a good thing to move it to shared directory to keep a single source of truth.