Don't bother comparing constructor when deps are not provided #14594
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
acdlite
force-pushed
the
never-memoize-if-no-deps
branch
2 times, most recently
from
8714c53
to
c0d53ae
Compare
sebmarkbage
reviewed
Jan 15, 2019
sebmarkbage
reviewed
Jan 15, 2019
sebmarkbage
reviewed
Jan 15, 2019
sebmarkbage
reviewed
Jan 15, 2019
|
ReactDOM: size: 0.0%, gzip: -0.0% Details of bundled changes.Comparing: 71b64d5...c7d8ebc react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
Generated by 🚫 dangerJS |
sebmarkbage
reviewed
Jan 15, 2019
sebmarkbage
reviewed
Jan 15, 2019
sebmarkbage
approved these changes
Jan 17, 2019
acdlite
force-pushed
the
never-memoize-if-no-deps
branch
from
04cb310
to
3ef1280
Compare
When no dependencies are passed to an effect hook, what we used to do is compare the effect constructor. If there was no change, then we would skip firing the effect. In practice, this is a useless optimization because the constructor will always be different when you pass an inline closure. And if you don't pass an inline closure, then you can't access any props or state. There are some edge cases where an effect that doesn't close over props or state could be useful, like reference counting the number of mounted components. But those are rare and can be addressed by passing an empty array of dependencies. By removing this "optimization," we can avoid retaining the constructor in the majority of cases where it's a closure that changes on every render. I made corresponding changes to the other hooks that accept dependencies, too (useMemo, useCallback, and useImperativeHandle).
It now includes the name of the hook in the message.
acdlite
force-pushed
the
never-memoize-if-no-deps
branch
from
3ef1280
to
c7d8ebc
Compare
gaearon
added a commit
to gaearon/react
that referenced
this pull request
Jan 18, 2019
gaearon
added a commit
that referenced
this pull request
Jan 18, 2019
| @@ -57,6 +60,48 @@ function resolveCurrentlyRenderingComponent(): Object { | |||
| return currentlyRenderingComponent; | |||
| } | |||
|
|
|||
| function areHookInputsEqual( | |||
There was a problem hiding this comment.
should this have been areHookDepsEqual?
jetoneza
pushed a commit
to jetoneza/react
that referenced
this pull request
Jan 23, 2019
…ok#14594) * Don't bother comparing constructor when deps are not provided When no dependencies are passed to an effect hook, what we used to do is compare the effect constructor. If there was no change, then we would skip firing the effect. In practice, this is a useless optimization because the constructor will always be different when you pass an inline closure. And if you don't pass an inline closure, then you can't access any props or state. There are some edge cases where an effect that doesn't close over props or state could be useful, like reference counting the number of mounted components. But those are rare and can be addressed by passing an empty array of dependencies. By removing this "optimization," we can avoid retaining the constructor in the majority of cases where it's a closure that changes on every render. I made corresponding changes to the other hooks that accept dependencies, too (useMemo, useCallback, and useImperativeHandle). * Improve hook dependencies warning It now includes the name of the hook in the message. * Nits
jetoneza
pushed a commit
to jetoneza/react
that referenced
this pull request
Jan 23, 2019
* Small tweaks to SSR to match facebook#14594 * Remove unnecessary comparison
Andarist
reviewed
Jan 29, 2019
| ); | ||
| } | ||
| } | ||
| for (let i = 0; i < prevDeps.length && i < nextDeps.length; i++) { |
There was a problem hiding this comment.
according to the previous check - those lengths should be the same, so shouldnt this iteration just assume that fact?
n8schloss
pushed a commit
to n8schloss/react
that referenced
this pull request
Jan 31, 2019
…ok#14594) * Don't bother comparing constructor when deps are not provided When no dependencies are passed to an effect hook, what we used to do is compare the effect constructor. If there was no change, then we would skip firing the effect. In practice, this is a useless optimization because the constructor will always be different when you pass an inline closure. And if you don't pass an inline closure, then you can't access any props or state. There are some edge cases where an effect that doesn't close over props or state could be useful, like reference counting the number of mounted components. But those are rare and can be addressed by passing an empty array of dependencies. By removing this "optimization," we can avoid retaining the constructor in the majority of cases where it's a closure that changes on every render. I made corresponding changes to the other hooks that accept dependencies, too (useMemo, useCallback, and useImperativeHandle). * Improve hook dependencies warning It now includes the name of the hook in the message. * Nits
n8schloss
pushed a commit
to n8schloss/react
that referenced
this pull request
Jan 31, 2019
* Small tweaks to SSR to match facebook#14594 * Remove unnecessary comparison
This was referenced Aug 31, 2019
This was referenced Sep 20, 2019
Open
[Snyk] Upgrade react-dom from 16.5.2 to 16.9.0
maxiplux/spring-security-react-ant-design-polls-app#3
Open
NMinhNguyen
referenced
this pull request
in enzymejs/react-shallow-renderer
Jan 29, 2020
* Small tweaks to SSR to match #14594 * Remove unnecessary comparison
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When no dependencies are passed to an effect hook, what we used to do is compare the effect constructor. If there was no change, then we would skip firing the effect. In practice, this is a useless optimization because the constructor will always be different when you pass an inline closure. And if you don't pass an inline closure, then you can't access any props or state.
There are some edge cases where an effect that doesn't close over props or state could be useful, like reference counting the number of mounted components. But those are rare and can be addressed by passing an empty array of dependencies.
By removing this "optimization," we can avoid retaining the constructor in the majority of cases where it's a closure that changes on every render.
I made corresponding changes to the other hooks that accept dependencies, too (useMemo, useCallback, and useImperativeHandle).