Skip to content
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

Should I rely on the dependency list check of useEffect hook? #3337

Closed
tranvansang opened this issue Oct 16, 2020 · 9 comments
Closed

Should I rely on the dependency list check of useEffect hook? #3337

tranvansang opened this issue Oct 16, 2020 · 9 comments

Comments

@tranvansang
Copy link
Contributor

tranvansang commented Oct 16, 2020

Consider the following code

useEffect(effect, [v]) // v can be undefined

I want to run the effect function only after the first render or when v changes.

My questions are

  1. Do I need to check whether v changes in effect (with some refs, carefully, to avoid additional deps) ? Or I can rely on react to check the change of values of the dependency list (i.e., the [v]).
  2. If the answer of 1. is the latter, is this behavior supposed to be changed in the near future? For e.g., when the concurrent mode is released.

From the official docs

Conditionally firing an effect

The default behavior for effects is to fire the effect after every completed render. That way an effect is always recreated if one of its dependencies changes.

It says if one of its dependencies changes, then, an effect is recreated. However, it does not mention the reverse: an effect is recreated if and only if one of its dependencies changes (or after the first render).

I only remember that in the early stage of hook, I read somewhere that the effect function can be run in some cases even without v's change, in order to optimize the memory, or in concurrent mode?? I really do not remember exactly and could not find out the source.

It would be very helpful if anyone can tell me how exactly React does, or refer to the related internal source code. I think there would be a special check of the first render, otherwise, when v is undefined after the first render, effect will not run.

If possible I think this question is worth to be added to the Hooks FAQ page.

@tranvansang tranvansang changed the title Should I rely on the dependency list check of react useEffect hook? Should I rely on the dependency list check of useEffect hook? Oct 16, 2020
@gaearon
Copy link
Member

gaearon commented Oct 16, 2020

Can you tell more about the use case? We do plan some features that would cause effects to re-fire in some cases more often. However, in those cases, it would usually be desirable for them to re-fire so I want to learn more about the specific case you're describing. What is your effect doing?

@tranvansang
Copy link
Contributor Author

tranvansang commented Oct 16, 2020

Lets see this example. When users click the "View All"/"View Less" button, we call toggleViewAll to toggle the viewAll state, and change documentElement.scrollTop to keep the current scroll position.

const [vewAll, setViewAll] = useState()
useLayoutEffect(() => {
  if (currentScrollTop.current === undefined) return
  document.documentElement.scrollTop = currentScrollTop.current
}, [viewAll])
  const toggleViewAll = () => {
    setViewAll((v) => {
      currentScrollTop.current = v
        ? undefined //dont' move the scroll when collapse
        : document.documentElement.scrollTop;
      return !v;
    });
  };

I am trying to find more examples. Because of what I asked in the main question, I have been always adding a check by refs in several effect functions where some actions should only be run when the state really changes.

Now I am confused if I should remove all of them and rely on react to do that.

@tranvansang
Copy link
Contributor Author

tranvansang commented Oct 16, 2020

Other examples

  • scroll to top when location.href changes
  • pull new resolved data from the server when location.href changes. Without a careful check, if location.href keeps the same, i.e., the user does not navigate anywhere, the last resolved data is replaced (even though its content doesn't change), which causes the data consumer to rerendered and losing all current state.

@gaearon
Copy link
Member

gaearon commented Oct 16, 2020

Hmm. The specific case where we're considering refiring is related to when the component is shown/hidden. Like a way to keep the component mounted but hidden on the screen instead of unmounting when the route changes. I think it would still be desirable to re-fire in this scenario because it matches view "appearing" / "disappearing", like on mount.

@tranvansang
Copy link
Contributor Author

tranvansang commented Oct 16, 2020

Let me summarize the scenario by the following state machine. We have these states

  • (1) unmounted, never rendered
  • (2) mounted
  • (3) rendered before but currently specially unmounted, and hidden instead of unmounted. As you said in the previous comment. This is a new state we never have seen before.

When the state changes

  • (1) -> (2): effect fires. All component's states are inited.
  • (2) -> (2): dispose fires, then effect fires
  • (2) -> (3): dispose fires
  • (3) -> (2): effect fires. All component's states keep unchanged.
  • (3) -> (1): do nothing
  • (1) -> (3): never happen
  • (3) -> (3): never happen
  • (1) -> (1): never happen

I think that what you are thinking desirable is considering (1) -> (2), (3) -> (2) are the same.

Let me think more about this and continue the discussion if I find anything at least in our projects.

@gaearon
Copy link
Member

gaearon commented Oct 16, 2020

(3) -> (3) could happen, this would be a regular update (while it's hidden). The rest makes sense. To be clear, this is still pretty hypothetical, but it's something we're beginning to experiment with. We wouldn't enable these semantics in old mode in either case, but it might be useful for newer features.

@tranvansang
Copy link
Contributor Author

the idea sounds interesting, I have never been thinking about (3), like maintain an unmounting state of a component. For a very unopinionated project like react, this is a hard decision to choose, IMHO.

Let me share what I am thinking about, and hope that this can help. Regarding what the users consider, when (3) is the current state, about the appearance of the component.

  • only (i) The component should be unmounted. Then everything is fine with the effect function, except that the previous states are different (inited, v.s., changed). We have the benefit of a faster re-rendering old component, but more memory consumed.
  • only (ii) The component should be mounted. This sounds not good because the dispose function is called unexpectedly.
  • can be (i) or (ii)

@tranvansang
Copy link
Contributor Author

tranvansang commented Oct 16, 2020

these types of effect function are desired

  • (a) only runs after the first render
  • (b) only runs after some changes from after the second render

Assume only (i) is the answer for the last comment, after the transition (3) -> (2)

  • If I do manual refs check in the effect function, (a) doesn't fire. (b) doesn't fire. Actually, this depends on the behavior of the useRef.
  • If I rely upon react to handle dependencies check internally, (a) fires. (b) fires.

In conclusion, I think everything is safe in at least in our situation. In other words, it is safe to rely on react to check the dependencies' changes, at least where (a) and (b) are not distinguished, and there is no side effect in the render.

@tranvansang
Copy link
Contributor Author

tranvansang commented Apr 16, 2022

With the new StrictMode behavior in react 18, useEffect is fired unconditionally in the second render, even though the deps array does not change.

My usecase is below

const useData = (params) => {
  useEffect(() => {
    // if data is not available in cache fetch data
    if (store.has(params)) prefetchData(params)
  })
  const initedRef = useRef(false)
  useEffect(() => {
    // if we can rely on the condition that useEffect ONLY fires in the first render or when the deps array changes
    // this implementation works as expected
    if (!initedRef.current) return
    initedRef.current = true
    //refetch data ONLY when params changes
    prefetchData(params)
  }. [params])
}

Suppose that the StrictMode's purpose is to make sure that even if the second effect runs, it shouldn't cause any problem.
However, in fact, it causes the hydration to fail, as described in this issue facebook/react#24388.

My current solution is to manually check if the deps array really changes before calling prefetchData(params) in the second effect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants