Skip to content

Design decision: why do we need the stale closure problem in the first place? #16956

Open
@slorber

Description

@slorber

Hi,

I initially asked this on Twitter and @gaearon suggested me to open an issue instead.
The original thread is here: https://twitter.com/sebastienlorber/status/1178328607376232449?s=19
More easy to read here: https://threadreaderapp.com/thread/1178328607376232449.html
But will try to make this issue more clear and structured about my args and questions.

Don't get me wrong, I really like hooks, but wonder if we can't have smarter abstractions and official patterns that make dealing with them more easy for authors and consumers.


Workaround for the stale closure

After using hooks for a while, and being familiar with the stale closure problem, I don't really understand why we need to handle closure dependencies, instead of just doing something like the following code, which always executes latest provided closure (capturing fresh variables)

image

Coupling the dependencies of the closure and the conditions to trigger effect re-execution does not make much sense to me. For me it's perfectly valid to want to capture some variables in the closure, yet when those variables change we don't necessarily want to re-execute.

There are many cases where people are using refs to "stabilize" some value that should not trigger re-execution, or to access fresh values in closures.

Examples in major libs includes:

Also @Andarist (who maintains a few important React libs for a while):

image

We often find in such codebase the "useIsomorphicLayoutEffect" hook which permits to ensure that the ref is set the earliest, and try to avoid the useLayoutEffect warning (see https://gist.github.com/gaearon/e7d97cdf38a2907924ea12e4ebdf3c85). What we are doing here seems unrelated to layout and makes me a bit uncomfortable btw.

Do we need an ESLint rule?

The ESLint rule looks to me only useful to avoid the stale closure problem. Without the stale closure problem (which the trick above solves), you can just focus on crafting the array/conditions for effect re-execution and don't need ESLint for that.

Also this would make it easier to wrap useEffect in userland without the fear to exposing users to stale closure problem, because eslint plugin won't notice missing dependencies for custom hooks.

Here's some code for react-navigation (alpha/v5). To me this is weird to have to ask the user to "useCallback" just to stabilize the closure of useFocusEffect, just to ensure the effect only runs on messageId change.

image

Not sure to understand why we can't simply use the following instead. For which I don't see the point of using any ESLint rule. I just want the effect to run on messageId change, this is explicit enough for me and there's no "trap"

image

I've heard that the React team recommends rather the later, asking the user to useCallback, instead of building custom hooks taking a dependency array, why exactly? Also heard that the ESLint plugin now was able to detect missing deps in a custom hook, if you add the hook name to ESLint conf. Not, sure what to think we are supposed to do in the end.

Are we safe using workarounds?

It's still a bit hard for me to be sure which kind of code is "safe" regarding React's upcoming features, particularly Concurrent Mode.

If I use the useEffectSafe above or something equivalent relying on refs, I am safe and future proof?

If this is safe, and makes my life easier, why do I have to build this abstraction myself?

Wouldn't it make sense to make this kind of pattern more "official" / documented?

I keep adding this kind of code to every project I work with:

const useGetter = <S>(value: S): (() => S) => {
  const ref = useRef(value);
  useIsomorphicLayoutEffect(() => {
    ref.current = value;
  });
  return useCallback(() => ref.current, [ref]);
};

(including important community projects like react-navigation-hooks)

Is it a strategy to teach users?

Is it a choice of the React team to not ship safer abstractions officially and make sure the users hit the closure problem early and get familiar with it?

Because anyway, even when using getters, we still can't prevent the user to capture some value. This has been documented by @sebmarkbage here with async code, even with a getter, we can't prevent the user to do things like:

onMount(async () => {
  let isEligible = getIsEligible();
  let data = await fetch(...);
  // at this point, isEligible might has changed: we should rather use `getIsEligible()` again instead of storing a boolean in the closure (might depend on the usecase though, but maybe we can imagine isEligible => isMounted)
  if (isEligible) {
    doStuff(data);
  }
});

As far as I understand, this might be the case:

So you can easily get into the same situation even with a mutable source value. React just makes you always deal with it so that you don't get too far down the road before you have to refactor you code to deal with these cases anyway. I'm really glad how well the React community has dealt with this since the release of hooks because it really sets us up to predictably deal with more complex scenario and for doing more things in the future.

A concrete problem

A react-navigation-hooks user reported that his effect run too much, using the following code:

image

In practice, this is because react-navigation core does not provide stable navigate function, and thus the hooks too. The core does not necessarily want to "stabilize" the navigate function and guarantee that contract in its API.

It's not clear to me what should I do, between officially stabilizing the navigate function in the hooks project (relying on core, so core can still return distinct navigate functions), or if I should ask the user to stabilize the function himself in userland, leading to pain and boilerplate for many users trying to use the API.

I don't understand why you can't simply dissociate the closure dependencies to the effect's triggering, and simply omitting the navigate function here:

image

What bothers me is that somehow as hooks lib authors we now have to think about whether what we return to the user is stable or not, ie safe to use in an effect dependency array without unwanted effect re-executions.

Returning a stable value in v1 and unstable in v2 is a breaking change that might break users apps in nasty ways, and we have to document this too in our api doc, or ask the user to not trust us, and do the memoization work themselves, which is quite error prone and verbose. Now as lib authors we have to think not only about the inputs/outputs, but also about preserving identities or not (it's probably not a new problem, because we already need to in userland for optimisations anyway).

Asking users to do this memoization themselves is error prone and verbose. And intuitively some people will maybe want to useMemo (just because of the naming) which actually can tricks them by not offering the same guarantees than useCallback.

A tradeoff between different usecases in the name of a consistent API?

@satya164 also mentionned that there are also usecases where the ESLint plugin saved him more than once because he forgot some dependency, and for him, it's more easy to fix an effect re-executing too much than to find out about some cached value not updating.

I see how the ESLint plugin is really handy for usecases such as building a stable object to optimize renders or provide a stable context value.

But for useEffect, when capturing functions, sometimes executing 2 functions with distinct identities actually lead to the same result. Having to add those functions to dependencies is quite annoying in such case.

But I totally understand we want to guarantee some kind of consistency across all hooks API.

Conclusion

I try to understand some of the tradeoffs being made in the API. Not sure to understand yet the whole picture, and I'm probably not alone.

@gaearon said to open an issue with a comment: It's more nuanced. I'm here to discuss all the nuances if possible :)

What particularly bothers me currently is not necessarily the existing API. It's rather:

  • the dogmatism of absolutely wanting to conform the ESLint rules (for which I don't agree with for all usecases). Currently I think users are really afraid to not follow the rules.
  • the lack of official patterns on how we are supposed to handle some specific hooks cases. And I think the "getter" pattern should be a thing that every hooks users know about and learn very early. Eventually adding such pattern in core would make it even more visible. Currently it's more lib authors and tech leads that all find out about this pattern in userland with small implementation variations.

Those are the solutions that I think of. As I said I may miss something important and may change my opinions according to the answers.

As an author of a few React libs, I feel a bit frustrated to not be 100% sure what kind of API contract I should offer to my lib's users. I'm also not sure about the hooks patterns I can recommend or not. I plan to open-source something soon but don't even know if that's a good idea, and if it goes in the direction the React team want to go with hooks.

Thanks

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions