From 9b3b430e1a9515f9ccb955d0aafc1dda177312cd Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Mon, 3 Apr 2023 08:40:49 -1000 Subject: [PATCH 1/8] Add some frequently asked questions --- contributingGuides/STYLE.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/contributingGuides/STYLE.md b/contributingGuides/STYLE.md index 9d94623f0e33..1dd5ee7d92df 100644 --- a/contributingGuides/STYLE.md +++ b/contributingGuides/STYLE.md @@ -618,6 +618,24 @@ There are several ways to use and declare refs and we prefer the [callback metho We love React and learning about all the new features that are regularly being added to the API. However, we try to keep our organization's usage of React limited to the most stable set of features that React offers. We do this mainly for **consistency** and so our engineers don't have to spend extra time trying to figure out how everything is working. That said, if you aren't sure if we have adopted something please ask us first. +# React Hooks: Frequently Asked Questions + +## Are Hooks a Replacement for HOCs or Render Props? + +In most cases, a custom hook is a better pattern to use than an HOC or Render Prop. They are easier to create, understand, use and document. However, there might still be a case for a HOC e.g. if you have a component that abstracts some conditional rendering logic. + +## Where should I put my inline functions? I'm getting a lint error related to `jsx-no-bind`... + +If your inline function does not have any dependencies (i.e. props or state that it depends on) it can be removed from the component and moved to the top of the file. If it does have dependencies, then we should wrap it in `useCallback()` and pass the dependencies as an argument. At one time, we questioned whether there is some performance penalty to using `useCallback()` as a pre-optimization and the conclusion was that there is generally a higher potential cost to not using it vs. using it. + +## Is there an equivalent to `componentDidUpdate()` when using hooks? + +The short answer is no. A longer answer is that sometimes we need to check not only that a dependency has changed, but how it has changed in order to run a side effect. For example, a prop had a value of an empty string on a previous render, but now is non-empty. The generally accepted practice is to store the "previous" value in a `ref` so the comparison can be made in a `useEffect()` call. + +## What is the `exhaustive-deps` lint rule? Can I ignore it? + +A `useEffect()` that does not include referenced props or state in its dependency array is [usually a mistake](https://legacy.reactjs.org/docs/hooks-faq.html#is-it-safe-to-omit-functions-from-the-list-of-dependencies) as often we want effects to re-run when those dependencies change. However, there are some cases where we might actually only want to re-run the effect when only some of those dependencies change. We determined the best practice here should be to allow disabling the “next line” with a comment `//eslint-disable-next-line react-hooks/exhaustive-deps` and an additional comment explanation so the next developer can understand why the rule was not used. + # Onyx Best Practices [Onyx Documentation](https://github.com/expensify/react-native-onyx) From 5f96597c76211344472c73b7b365280feef11bd1 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Wed, 5 Apr 2023 13:37:29 -1000 Subject: [PATCH 2/8] Add some notes about useState()` --- contributingGuides/STYLE.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contributingGuides/STYLE.md b/contributingGuides/STYLE.md index 1dd5ee7d92df..fb7c71cb02f1 100644 --- a/contributingGuides/STYLE.md +++ b/contributingGuides/STYLE.md @@ -628,6 +628,10 @@ In most cases, a custom hook is a better pattern to use than an HOC or Render Pr If your inline function does not have any dependencies (i.e. props or state that it depends on) it can be removed from the component and moved to the top of the file. If it does have dependencies, then we should wrap it in `useCallback()` and pass the dependencies as an argument. At one time, we questioned whether there is some performance penalty to using `useCallback()` as a pre-optimization and the conclusion was that there is generally a higher potential cost to not using it vs. using it. +## Why does `useState()` sometimes get initialized with a function? + +React saves the initial state once and ignores it on the next renders. However, if you pass the result of a function to `useState()` or call a function directly e.g. `useState(doExpensiveThings())` it will *still run on every render*. This can hurt performance depending on what work the function is doing. As an optimization, we can pass an initializer function instead of a value e.g. `useState(doExpensiveThings)` or `useState(() => doExpensiveThings())`. + ## Is there an equivalent to `componentDidUpdate()` when using hooks? The short answer is no. A longer answer is that sometimes we need to check not only that a dependency has changed, but how it has changed in order to run a side effect. For example, a prop had a value of an empty string on a previous render, but now is non-empty. The generally accepted practice is to store the "previous" value in a `ref` so the comparison can be made in a `useEffect()` call. From d91a7bfa4c63a07371b77a84e3de862dc6f0f68c Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Tue, 25 Apr 2023 07:44:37 -1000 Subject: [PATCH 3/8] Add some notes about useCallback() vs useMemo() --- contributingGuides/STYLE.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contributingGuides/STYLE.md b/contributingGuides/STYLE.md index fb7c71cb02f1..dac0f031deaa 100644 --- a/contributingGuides/STYLE.md +++ b/contributingGuides/STYLE.md @@ -636,6 +636,10 @@ React saves the initial state once and ignores it on the next renders. However, The short answer is no. A longer answer is that sometimes we need to check not only that a dependency has changed, but how it has changed in order to run a side effect. For example, a prop had a value of an empty string on a previous render, but now is non-empty. The generally accepted practice is to store the "previous" value in a `ref` so the comparison can be made in a `useEffect()` call. +## Are `useCallback()` and `useMemo()` basically the same thing? + +No! Is it easy to confuse `useCallback()` with a memoization helper like `_.memoize()` or `useMemo()`. It is really not the same at all. [`useCallback()` will return a cached function _definition_](https://react.dev/reference/react/useCallback) and will not save us any computational cost of running that function. So, if you are wrapping something in a `useCallback()` and then calling it in the render then it is better to use `useMemo()` to cache the actual **result** of calling that function and use it directly in the render. + ## What is the `exhaustive-deps` lint rule? Can I ignore it? A `useEffect()` that does not include referenced props or state in its dependency array is [usually a mistake](https://legacy.reactjs.org/docs/hooks-faq.html#is-it-safe-to-omit-functions-from-the-list-of-dependencies) as often we want effects to re-run when those dependencies change. However, there are some cases where we might actually only want to re-run the effect when only some of those dependencies change. We determined the best practice here should be to allow disabling the “next line” with a comment `//eslint-disable-next-line react-hooks/exhaustive-deps` and an additional comment explanation so the next developer can understand why the rule was not used. From e220bee62c506618132145fce56e27905410c83e Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Tue, 25 Apr 2023 08:49:44 -1000 Subject: [PATCH 4/8] Update some examples and standardize on function syntax --- contributingGuides/STYLE.md | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/contributingGuides/STYLE.md b/contributingGuides/STYLE.md index dac0f031deaa..b9ff75ca928f 100644 --- a/contributingGuides/STYLE.md +++ b/contributingGuides/STYLE.md @@ -302,20 +302,24 @@ const {firstName, lastName} = state; ... // Bad -const UserInfo = ({name, email}) => ( - - Name: {name} - Email: {email} - -); +function UserInfo({name, email}) { + return ( + + Name: {name} + Email: {email} + + ); +} // Good -const UserInfo = props => ( - - Name: {props.name} - Email: {props.email} - -); +function UserInfo(props) { + return ( + + Name: {props.name} + Email: {props.email} + + ); +} ``` ## Named vs Default Exports in ES6 - When to use what? @@ -563,7 +567,7 @@ When writing a function component you must ALWAYS add a `displayName` property a ```javascript - const Avatar = (props) => {...}; + function Avatar(props) {...}; Avatar.propTypes = propTypes; Avatar.defaultProps = defaultProps; @@ -644,6 +648,10 @@ No! Is it easy to confuse `useCallback()` with a memoization helper like `_.memo A `useEffect()` that does not include referenced props or state in its dependency array is [usually a mistake](https://legacy.reactjs.org/docs/hooks-faq.html#is-it-safe-to-omit-functions-from-the-list-of-dependencies) as often we want effects to re-run when those dependencies change. However, there are some cases where we might actually only want to re-run the effect when only some of those dependencies change. We determined the best practice here should be to allow disabling the “next line” with a comment `//eslint-disable-next-line react-hooks/exhaustive-deps` and an additional comment explanation so the next developer can understand why the rule was not used. +## Should I declare my components with arrow functions (`const`) or the `function` keyword? + +There are pros and cons of each, but ultimately we have standardized on using the `function` keyword to align things more with modern React conventions. There are also some minor cognitive overhead benefits in that you don't need to think about adding and removing brackets when encountering an implicit return. The `function` syntax also has the benefit of being able to be hoisted where arrow functions do not. + # Onyx Best Practices [Onyx Documentation](https://github.com/expensify/react-native-onyx) From efec3607e6faaaebcdef1995feeb9616ec48e842 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Fri, 2 Jun 2023 06:57:33 -1000 Subject: [PATCH 5/8] Update guidance on useCallback() --- contributingGuides/STYLE.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contributingGuides/STYLE.md b/contributingGuides/STYLE.md index b9ff75ca928f..599865977638 100644 --- a/contributingGuides/STYLE.md +++ b/contributingGuides/STYLE.md @@ -628,9 +628,9 @@ We love React and learning about all the new features that are regularly being a In most cases, a custom hook is a better pattern to use than an HOC or Render Prop. They are easier to create, understand, use and document. However, there might still be a case for a HOC e.g. if you have a component that abstracts some conditional rendering logic. -## Where should I put my inline functions? I'm getting a lint error related to `jsx-no-bind`... +## Should I wrap all my inline functions with `useCallback()` or move them out of the component if they have no dependencies? -If your inline function does not have any dependencies (i.e. props or state that it depends on) it can be removed from the component and moved to the top of the file. If it does have dependencies, then we should wrap it in `useCallback()` and pass the dependencies as an argument. At one time, we questioned whether there is some performance penalty to using `useCallback()` as a pre-optimization and the conclusion was that there is generally a higher potential cost to not using it vs. using it. +The answer depends on whether you need a stable reference for the function. If there are no dependencies, you could move the function out of the component. If there are dependencies, you could use `useCallback()` to ensure the reference updates only when the dependencies change. However, it's important to note that using `useCallback()` may have a performance penalty, although the trade-off is still debated. It's recommended to follow the guidance in the [React documentation](https://react.dev/reference/react/useCallback#should-you-add-usecallback-everywhere) and add the optimization only if necessary. Leave a code comment explaining the reasoning behind the chosen optimization to aid reviewers and future contributors. ## Why does `useState()` sometimes get initialized with a function? From a34ad0cfdb9dbe3e4aaf199b19af87361da7b430 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Fri, 2 Jun 2023 06:58:16 -1000 Subject: [PATCH 6/8] Add useMemo() --- contributingGuides/STYLE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contributingGuides/STYLE.md b/contributingGuides/STYLE.md index 599865977638..3833f477e13d 100644 --- a/contributingGuides/STYLE.md +++ b/contributingGuides/STYLE.md @@ -630,7 +630,7 @@ In most cases, a custom hook is a better pattern to use than an HOC or Render Pr ## Should I wrap all my inline functions with `useCallback()` or move them out of the component if they have no dependencies? -The answer depends on whether you need a stable reference for the function. If there are no dependencies, you could move the function out of the component. If there are dependencies, you could use `useCallback()` to ensure the reference updates only when the dependencies change. However, it's important to note that using `useCallback()` may have a performance penalty, although the trade-off is still debated. It's recommended to follow the guidance in the [React documentation](https://react.dev/reference/react/useCallback#should-you-add-usecallback-everywhere) and add the optimization only if necessary. Leave a code comment explaining the reasoning behind the chosen optimization to aid reviewers and future contributors. +The answer depends on whether you need a stable reference for the function. If there are no dependencies, you could move the function out of the component. If there are dependencies, you could use `useCallback()` to ensure the reference updates only when the dependencies change. However, it's important to note that using `useCallback()` may have a performance penalty, although the trade-off is still debated. It's recommended to follow the guidance in the [React documentation](https://react.dev/reference/react/useCallback#should-you-add-usecallback-everywhere) and add the optimization only if necessary. Leave a code comment explaining the reasoning behind any usage of `useCallback()` or `useMemo()` to aid reviewers and future contributors. ## Why does `useState()` sometimes get initialized with a function? From d4e23fce219d51e1444a8f0343c5907add60ece5 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Fri, 2 Jun 2023 07:01:41 -1000 Subject: [PATCH 7/8] suggest adding comments when there is lack of clarity --- contributingGuides/STYLE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contributingGuides/STYLE.md b/contributingGuides/STYLE.md index 3833f477e13d..51443f4ac20a 100644 --- a/contributingGuides/STYLE.md +++ b/contributingGuides/STYLE.md @@ -630,7 +630,7 @@ In most cases, a custom hook is a better pattern to use than an HOC or Render Pr ## Should I wrap all my inline functions with `useCallback()` or move them out of the component if they have no dependencies? -The answer depends on whether you need a stable reference for the function. If there are no dependencies, you could move the function out of the component. If there are dependencies, you could use `useCallback()` to ensure the reference updates only when the dependencies change. However, it's important to note that using `useCallback()` may have a performance penalty, although the trade-off is still debated. It's recommended to follow the guidance in the [React documentation](https://react.dev/reference/react/useCallback#should-you-add-usecallback-everywhere) and add the optimization only if necessary. Leave a code comment explaining the reasoning behind any usage of `useCallback()` or `useMemo()` to aid reviewers and future contributors. +The answer depends on whether you need a stable reference for the function. If there are no dependencies, you could move the function out of the component. If there are dependencies, you could use `useCallback()` to ensure the reference updates only when the dependencies change. However, it's important to note that using `useCallback()` may have a performance penalty, although the trade-off is still debated. You might choose to do nothing at all if there is no obvious performance downside to declaring a function inline. It's recommended to follow the guidance in the [React documentation](https://react.dev/reference/react/useCallback#should-you-add-usecallback-everywhere) and add the optimization only if necessary. If it's not obvious why such an optimization (i.e. `useCallback()` or `useMemo()`) would be used, leave a code comment explaining the reasoning to aid reviewers and future contributors. ## Why does `useState()` sometimes get initialized with a function? From 28fc22b2ace6ea68d70d5a2c2e635d1bc91d370d Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Fri, 2 Jun 2023 11:03:40 -1000 Subject: [PATCH 8/8] Update docs --- contributingGuides/STYLE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contributingGuides/STYLE.md b/contributingGuides/STYLE.md index 51443f4ac20a..11af5377b567 100644 --- a/contributingGuides/STYLE.md +++ b/contributingGuides/STYLE.md @@ -642,7 +642,7 @@ The short answer is no. A longer answer is that sometimes we need to check not o ## Are `useCallback()` and `useMemo()` basically the same thing? -No! Is it easy to confuse `useCallback()` with a memoization helper like `_.memoize()` or `useMemo()`. It is really not the same at all. [`useCallback()` will return a cached function _definition_](https://react.dev/reference/react/useCallback) and will not save us any computational cost of running that function. So, if you are wrapping something in a `useCallback()` and then calling it in the render then it is better to use `useMemo()` to cache the actual **result** of calling that function and use it directly in the render. +No! It is easy to confuse `useCallback()` with a memoization helper like `_.memoize()` or `useMemo()` but they are really not the same at all. [`useCallback()` will return a cached function _definition_](https://react.dev/reference/react/useCallback) and will not save us any computational cost of running that function. So, if you are wrapping something in a `useCallback()` and then calling it in the render then it is better to use `useMemo()` to cache the actual **result** of calling that function and use it directly in the render. ## What is the `exhaustive-deps` lint rule? Can I ignore it?