Add more info to FAQ about deps #1815
Merged
Conversation
This file contains hidden or 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
gaearon
commented
Mar 12, 2019
| useEffect(() => { | ||
| function handleStatusChange(status) { |
There was a problem hiding this comment.
I moved these since it's harder to tell when it's safe, and we should probably just teach people to put them inside effects as a default.
There was a problem hiding this comment.
I agree. Functions are generally only called within the effect, so prefer to teach this as the default. As a result a value and not a function will be the effect's dependency, which is slightly easier to understand for beginners.
|
Deploy preview for reactjs ready! Built with commit 7639a38 |
content/docs/hooks-faq.md
Outdated
| @@ -362,6 +365,44 @@ It's possible that in the future React will provide a `usePrevious` Hook out of | |||
|
|
|||
| See also [the recommended pattern for derived state](#how-do-i-implement-getderivedstatefromprops). | |||
|
|
|||
| ### Why am I seeing stale props or state inside my function? {#why-am-i-seeing-stale-props-or-state-inside-my-function} | |||
|
|
|||
| Any function inside a component, including event handlers and effects, “sees” the props and state from the render it was defined in. For example, consider code like this: | |||
There was a problem hiding this comment.
A few suggestions.
- "sees" is clear enough for me, but due to the quotes maybe a little imprecise.
- Functions are defined once, but are potentially created multiple times (during renders).
Suggested change
| Any function inside a component, including event handlers and effects, “sees” the props and state from the render it was defined in. For example, consider code like this: | |
| Any function inside a component, including event handlers and effects, contain ("sees") the props and state from the render it was created in. For example, consider code like this: |
sophiebits
reviewed
Mar 13, 2019
content/docs/hooks-effect.md
Outdated
| > | ||
| >If you want to run an effect and clean it up only once (on mount and unmount), you can pass an empty array (`[]`) as a second argument. This tells React that your effect doesn't depend on *any* values from props or state, so it never needs to re-run. This isn't handled as a special case -- it follows directly from how the inputs array always works. While passing `[]` is closer to the familiar `componentDidMount` and `componentWillUnmount` mental model, we suggest not making it a habit because it often leads to bugs, [as discussed above](#explanation-why-effects-run-on-each-update). Don't forget that React defers running `useEffect` until after the browser has painted, so doing extra work is less of a problem. | ||
| >We provide an [`exhaustive-deps`](https://github.com/facebook/react/issues/14920) ESLint rule as a part of the [`eslint-plugin-react-hooks`](https://www.npmjs.com/package/eslint-plugin-react-hooks#installation) package. It warns when dependencies are specified incorrectly and suggests a fix. |
There was a problem hiding this comment.
We recommend using the … as part of our …
content/docs/hooks-faq.md
Outdated
| ```js | ||
| useEffect(() => { | ||
| doSomething(); | ||
| }, []); // 🔴 This is not safe |
There was a problem hiding this comment.
Can you change this example so that it clearly comes from props and state? Since it is safe to call global functions.
Maybe by showing
function doSomething() {
console.log(someProp);
}
in component scope.
content/docs/hooks-faq.md
Outdated
|
|
||
| Let's see why this matters. | ||
|
|
||
| If you specify a [list of dependencies](/docs/hooks-reference.html#conditionally-firing-an-effect) as the last argument to `useEffect`, `useMemo`, `useCallback`, or `useImperativeHandle`, it must include all values used inside that participate in the React data flow. That includes props, state, or anything derived from them. |
There was a problem hiding this comment.
props, state, and anything derived from them.
Co-Authored-By: gaearon <[email protected]>
Co-Authored-By: gaearon <[email protected]>
Co-Authored-By: gaearon <[email protected]>
Co-Authored-By: gaearon <[email protected]>
Co-Authored-By: gaearon <[email protected]>
|
I'll get this in for now. I'd love if someone can figure out a way to make it shorter but at least it's in the docs now. |
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.
This addresses the common confusion points around the deps array and optimizing effects, and suggests concrete fixes. It might be too wordy but I figured it's probably worth including.
Open to edits.