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.

I'm a little skeptical of this. It seems like you're saying that it's okay for certain values that your function operates on to be totally arbitrary. Can you provide a couple of concrete cases when this is true?

Are we safe using workarounds?

This is a really long GH issue, but I wanted to respond to this particular question. There is a potential serious bug when it come to using effects and refs to wrap closures, given React's current hooks primitives.

First, here's the similar pattern I've seen used (within Facebook too):

function useDynamicCallback(callback) {
  const ref = useRef(callback);

  useIsomorphicLayoutEffect(() => {
    ref.current = callback;
  }, [callback]);

  return useCallback((...args) => ref.current(...args), []);
}

This has a pretty serious flaw if you were to ever pass the callback it creates as a prop to a child component. For example:

function Example(props) {
  const callback = useDynamicCallback(props.callback);
  return <Child callback={callback} />;
}

function Child({ callback }) {
  useLayoutEffect(() => {
    // Child effects are run before parent effects!
    // This means the callback ref has not yet been updated-
    // which means that it still points to the previous version,
    // with stale values and potentially different behavior.
    // This function call will probably cause problems!
    callback();
  }, []);
}

Note that useEffectSafe shown in the issue description above uses useEffect. This is okay since it's self contained, but if you were to use useEffect for a generic callback wrapper- it would be even more likely to trigger a bug like the one above- since all useLayoutEffect functions would run before useEffect updated the ref.

I think that implementing a hook like this correctly would require React to add an API like useMutationEffect (see #14336).

I'm a little skeptical of this. It seems like you're saying that it's okay for certain values that your function operates on to be totally arbitrary. Can you provide a couple of concrete cases when this is true?

From what I understand @slorber's intention - it's not about operating on arbitrary values. He just wants to operate on "current" props/state (mostly), but this with hooks leads to a memoization dance (especially when developing libraries where we don't want to assume a position in component hierarchy etc so it seems safer to provide/accept memoized callbacks. So this is just a discussion about recommended patterns and how we could possibly try to avoid this problem.

This has a pretty serious flaw if you were to ever pass the callback it creates as a prop to a child component.

Thanks for a compelling example! Seems like indeed providing a "memoized" callback with this technique is currently a no-go. Consuming stuff just for internal hook's usage seems OK though, right?

From what I understand @slorber's intention - it's not about operating on arbitrary values. He just wants to operate on "current" props/state (mostly)

I'm not sure this is my understanding of the original claim:

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.

If you don't want to re-capture when values change, then the values you'll have closed over won't be "current". But maybe some concrete examples would help clarify if there's a misunderstanding?

Consuming stuff just for internal hook's usage seems OK though, right?

If you're referring to an example like useEffectSafe above, where the ref is only used by another effect within the same hook, I think that seems okay.

I think you can probably accomplish the same thing just using a utility like memoize-one btw (assuming the callback accepts props).

There's a bit to unpack here and your original comment doesn't quite line up with how I see this progression so I'll respond in a different format so pardon me if I don't answer some questions directly.

This is very fair feedback and a general pain point people have right now. In fact, this is basically Vue's whole point with their version of Hooks to support mutable containers being captured by closures. It's a valid approach but it does cause a lot of downstream effects. So we're hoping we can achieve a different approach over time.

Not Just Closures

Let me first point out that (same as in classes), this is not limited to closures. The same thing happens with objects too.

<Foo style={{foo}} />

This captures stale data in the same way as closures do.

<Foo onClick={() => foo} />

The difference is that closures might be a bit more opaque. You might be able to do a shallow comparison on an unknown object but it happens there too.

It just happens that there are common patterns that happen to use closures as the most idiomatic API. It's possible that these patterns can be redesigned to avoid closures.

Composability

The most common way this happens is by passing something through props. This creates a copy of the value. This decoupling is how we are able to create many composable abstractions. It is very common and idiomatic in React so we can't dismiss that this is going to be the most common form. This has indirect downstream effects because if you can't rely on mutation propagating in one intermediate level, you can't rely on it deeper neither.

The primary goal of React's API is to define an API boundary between abstractions. A common language that has certain properties that you can rely on. That's why it's important that it's clear when certain values have certain properties. E.g. refs provide a common language to talk about a "box" that may contain mutable values for interop with imperative systems.

If a Hook isn't responsive to new values, that break such a contract. So in general, it would be a bad idea to let that pattern spread.

useEventCallback

The useEventCallback concept is basically something like this:

useEventCallback = (fn) => {
  let ref = useRef(fn);
  useEffect(() => ref.current = fn);
  return useCallback(() => ref.current.apply(this, arguments));
}

I actually came up with this very early in the process, before Hooks was a thing.

However it has some bad qualities that lead me to want to strongly discourage this pattern.

• useEffect is not always safe because there's a potential gap between commit and useEffect. Many people know that, but what people don't know is that useLayoutEffect also isn't completely safe because sometimes these callbacks are used by other effects deeper in the tree that fire first. useLayoutEffect(() => props.onCallback()); and this will have the stale value in this case but a plain useCallback wouldn't. So if this was a built in Hook, React would use a different phase to update it.

• In Concurrent Mode, we try hard to move as much as we can out of this critical "commit" phase. E.g. as much as possible should move to effects that don't block paint (useEffect) or the render phase which is async. This approach moves it to be synchronously blocking. This is probably mostly fine if this is a rare pattern. Once it is used everywhere, then it means that large updates will have a large synchronously blocking phase which undermines the responsiveness of the concurrent system.

• "Callbacks" in React are used in two ways. Either as a pure function during render or as part of a reducer, or an imperative method used as event handlers with side-effects. E.g. render props. Currently, simple functions and useCallback can be used for either. We'd need to be very clear that these functions cannot safely be called during render and separate the pure ones from the event callback ones. If this was a worth while strategy this could be doable.

If this was the only way to solve things, then we should adopt this upstream but I don't think this is ever actually the best solution available. So let's see what else we can do.

Keep Imperative Code Separate

I'd argue that the useEventCallback pattern is actually not necessary in general. It comes from using React state where you could be using refs.

If you get into this situation it's not because you have a nice purely functional model. It's because you have interop with imperative code and that code likely relies on subtle mutation anyway so it's not suitable to mix with React's deferred/batched/concurrent state.

E.g. imagine if your callback was using state like this:

let [state, setState] = useState(false);
let fnA = useCallback(() => {
  setState(true);
}, [state]);
let fnB = useCallback(() => {
  if (!state) doStuff();
}, [state]);

This is probably not great anyway since the setState can be batched and deferred so it might not update the second callback immediately.

If this instead used a ref then it wouldn't get invalidated all the time:

let ref = useRef(false);
let fnA = useCallback(() => {
  ref.current = true;
}, [ref]);
let fnB = useCallback(() => {
  if (!ref.current) doStuff();
});

Using a ref and imperative code is discouraged if you can avoid it, but if you're writing imperative code anyway then this might be the best way to solve it.

In this code, useCallback is sufficient because it won't invalidate as often as if the state was also captured there.

I think the issue is that React doesn't really make it easy to tell when you're entering imperative code and should be using a ref vs not. Also, it's not easy to read a ref in a render function easily. We have some work to do there.

Referential Identity == Best Practice

Now as lib authors we have to think not only about the inputs/outputs, but also about preserving identities or not

It seems to me that this is a natural evolving best practice just like other performance considerations. In libraries that work on the mutation model, you also have to think about if you're returning a reactive value that has its origin traced in an efficient way or if it's a derived copy that will be reevaluated.

I think we can do a lot more to make this automatic, but not if you're using patterns that aren't optimizable by us. For example, the useRef + useCallback pattern above is describing semantics that are easily optimizable in our current approach. Similarly passing down a stablee dispatcher instead of a callback that closes over state is best-practice that helps avoid the perf problems to begin with.

Strict Lint Rule

the dogmatism of absolutely wanting to conform the ESLint rules

I think this is actually quite important. We constantly get questions about why something didn't work and it's almost always because someone decided to break the rules in one place and the consequences snowballing elsewhere and then it's too late to refactor the code. It's really hard to know that it is safe to break it.

However, the strictness also is about promoting patterns that are automatable. The lint rule is just the first step. The next step we added was "autofixing" that added dependencies for you.

The next step is adding a compiler that adds all the memoization for you in the entire app. That way you don't have to think as much about useCallback and useMemo because we can infer it. I believe that this will alleviate a lot of the pain points since as a lib author you don't necessarily have to think about adding them, and you can assume that updates are much more fine grained in the system in general.

However, this can only automate what you're already expressing today. So if today's code isn't working then we need to either change our patterns, or shift the approach used by React. Breaking the rules, isn't just a convenience, it means that automation isn't safe.

Thanks @bvaughn for your answer. I'm happy you come up with this example because I felt there was something dangerous with my pattern but couldn't come up with a concrete example to illustrate that. I was mostly thinking about the potential execution order of hooks inside the same component, but not about execution order for parent/children relationships.

I'm a little skeptical of this. It seems like you're saying that it's okay for certain values that your function operates on to be totally arbitrary. Can you provide a couple of concrete cases when this is true?

Not really sure what's the best way to express this, but what I mean is sometimes, if the closure captures a variable, we don't really care if this variable was the render1 variable or render2 variable, even if the identity changes, because that variable may be "semantically equivalent" even if the identity is different.

const Screen = () => {
  const { navigate } = useNavigation();
  const [someBoolean, setSomeBoolean] = useState(false);
  useEffect(() => {
      if (someBoolean) {
          navigate(...)
      }
  },  [someBoolean])
}

Here, taking react-navigation as an example, the navigate function may change when the boolean goes from false (render1) to true (render2), yet, executing that function produces the same behavior, even if it's navigate1 or navigate2, doing navigate1("nextScreen") or navigate2("nextScreen") would lead to the exact same result. So in such case, even if navigate1 is executed, it does not really look like a problem to me. But I understand it might be tricky and wouldn't recommend this, because we can't really know it's safe without looking at implementation details (ie, does "navigate" capture any variables, ie navigate1 really equivalent to navigate2?).

That's why I'd use useEffectSafe, to ensure that navigate2("nextScreen") gets called on render2. I might as well use the getter pattern and do getNavigate()("nextScreen").

He just wants to operate on "current" props/state (mostly)

@Andarist yes that's what I meant, it looks to be a safer default to always operate on current values.

If you're referring to an example like useEffectSafe above, where the ref is only used by another effect within the same hook, I think that seems okay.

So as I understand it, useEffectSafe is ok because it guarantees the effect executes after the ref mutation, but my getter pattern might not work in all cases (see the example)

I think you can probably accomplish the same thing just using a utility like memoize-one btw (assuming the callback accepts props).

Not sure what you mean here. If navigate1 and navigate2 are safe to invert, I could use useConstant and store navigate1 and always use navigate1 for all renders

Thanks again for the example.

Didn't know about the useMutationEffect, that could be the job. Wonder about the naming. I'm not already a fan of "useLayoutEffect" which ends up being used for things totally unrelated to layout. As far as I understand, to implement the getter/dynamicCallback safely, we need something quite similar to what the dom provides with events, a top down capture phase, and a bottom-up bubbling phase. What about useCaptureEffect? Just thinking out loud.

Not Just Closures

This is, of course, true - but not that compelling argument (for me). Personally I don't mind implementing some fine-grained memoization where it is needed - it's rather easy to spot where you need it and you don't need all the time. The main problem is with callbacks which always suffer from this problem and you kinda need to memoize them constantly.

The problematic pattern mentioned here seems to be the getter pattern which comes really useful to use. I often find myself wanting to read a value somewhere down the tree, but don't want to keep that value in the state as it's not render-related - so this is mostly for being able to read it in things like event handlers. Arguably sometimes such use cases could be refactored to use dispatch from the parent component - but only if the own logic/render is not related to a particular action. Hoisting state is always not a feasible solution because we'd really want to keep the behavior implementations with the component in question, even if it's related to the "state" of a parent.

Keep Imperative Code Separate

This is a summary - could be probably included in the docs somewhere? This is also a pattern I had in mind when mentioning proposing this at code reviews etc with a combination of useEventCallback-like solution. The latter - as showcased here - can break when introducing parent-child relationships which is something to watch out for, unfortunately.

Also a sideways question - is useCallback's cache semantically guaranteed? useMemo's isn't (as stated in the docs) and useCallback is mentioned to be semantically equivalent to useMemo(() => callback) - but it's not obvious from the docs what are guarantees around useCallback's cache

Thanks for your answer and accepting by feedback @sebmarkbage , understand you see a different progression for your answer. It's also hard for me to formulate what I feel is wrong, the issue may look a bit messy :)

Not just closures

Not sure to understand what is stale here, considering foo is not stale in the first place, and it's regular JSX (ie, not returned by a useMemo closure or another fancy patterns)

<Foo style={{foo}} />

It's possible that these patterns can be redesigned to avoid closures.

Do you have an example of what you have in mind?

Composability

If a Hook isn't responsive to new values, that break such a contract. So in general, it would be a bad idea to let that pattern spread.

That seems legit. That's why I'm not sure the current API should be modified either, but still looking for a solution: that could be just a safe, recommended and widespread pattern to use, or maybe the useMutationEffect. I like how useEffect is consistent to other hooks in this regard and having exceptional behaviors would probably not help.

useEventCallback

useEffect is not always safe because there's a potential gap between commit and useEffect. Many people know that, but what people don't know is that useLayoutEffect also isn't completely safe because sometimes these callbacks are used by other effects deeper in the tree that fire first.

Thanks, @bvaughn illustrated that well and that's why I wanted to ask the question here: because I wasn't sure and couldn't find anybody to tell me ;)

In Concurrent Mode, we try hard to move as much as we can out of this critical "commit" phase.

The way I think about it is assigning a new value to a ref is fast enough to happen during this phase. But maybe it's more expensive than I think to generalize, and this is not what you want us to do?

"Callbacks" in React are used in two ways. Either as a pure function during render or as part of a reducer, or an imperative method used as event handlers with side-effects. E.g. render props. Currently, simple functions and useCallback can be used for either. We'd need to be very clear that these functions cannot safely be called during render and separate the pure ones from the event callback ones. If this was a worth while strategy this could be doable.

Not totally sure to understand what you mean.

The following pattern has already bitten me in the past, because of the child not re-rendering due to memo, when the parent count value changes.

function Example(props) {
  const [count, setCount]
  const getCount = useDynamicCallback(() => count);
  return <>
     <Child getCount={getCount} />;
     <Button onClick={() => setCount(c => c+1)}>Increment</Button>
   </>
}

React.memo(function Child({ getCount }) {
  return <div>{getCount()}</div>
})

I think it's already an antipattern to let data flow down the tree with getters instead of regular props, not sure it's totally related though to what you have in mind though...

Keep Imperative Code Separate

I think the issue is that React doesn't really make it easy to tell when you're entering imperative code and should be using a ref vs not. Also, it's not easy to read a ref in a render function easily. We have some work to do there.

Agree, and people probably try intuitively to fit their imperative stuff using declarative constructs.

About React-navigation we discussed to expose directly a ref through the API:

const Screen = () => {
  const navigationRef = useNavigationRef();
  const [someBoolean, setSomeBoolean] = useState(false);
  useEffect(() => {
      if (someBoolean) {
          navigationRef.current.navigate(...)
      }
  },  [navigationRef,someBoolean])
}

That would definitively work, but I have never seen such hooks API so far. Is it something you'd recommend?

Referential Identity == Best Practice

It seems to me that this is a natural evolving best practice just like other performance considerations. In libraries that work on the mutation model, you also have to think about if you're returning a reactive value that has its origin traced in an efficient way or if it's a derived copy that will be reevaluated.

Agree.

Strict Lint Rule

It's really hard to know that it is safe to break it.

That's true. The thing that bothers me is that sometimes, the code is working by breaking the rule, and fixing the rule might break the desired behavior (it's always possible to rewrite in a way that works and conforms to the rule, but still an important fact to consider).

The next step is adding a compiler that adds all the memoization for you in the entire app. That way you don't have to think as much about useCallback and useMemo because we can infer it.

Wow can't wait to see that in action :o

However, this can only automate what you're already expressing today. So if today's code isn't working then we need to either change our patterns, or shift the approach used by React. Breaking the rules, isn't just a convenience, it means that automation isn't safe.

It's not that it's not working, it's mostly that there are multiple ways to design an API and it's hard to see which design is the good choice.

For example in my react-navigation example:

function ReactComponent() {
  const { navigate } = useNavigation();
  useEffect(() => {
      if (someBoolean) {
          navigate(...)
      }
  },  [someBoolean,navigate])
}

It's not clear if it's my responsibility to make the navigate function stable and how. I would say yes, because it would simplify usage, but it involves some lib boilerplate. What I understand is that in the future this boilerplate could be automated?

In the meantime, I'd really like to know how you'd address this specific problem, between all the possible solutions.

Having some input/output examples of such a compiler would probably be very helpful to see what kind of code you would expect us to write manually until the compiler is ready.

Thanks again for your answer, really appreciate you took the time to answer this in depth ;)

A little bit off topic but I need some clarification : In every implementation of useDynamicCallback that I see, a useEffect or a custom useIsomorphicLayoutEffect is used, but why ? What is the point of updating the ref inside a use*Effect instead of just doing it outside like the following ?

function useDynamicCallback(callback) {
  const ref = useRef()
  ref.current = callback
  return useCallback((...args) => ref.current.apply(this, args), [])
}

@Baloche this is a good question and also thought about it. I suspect we don't want to update the ref too early. A dom event triggering this callback might be called in between a render call, and the dom commit of that render call. This could perhaps lead to the wrong callback being executed. For consistency, this seems to be more reliable to update in useLayoutEffect so that your new callback could only be used if the render updating it was actually applied to the dom.

Hi @sebmarkbage

About the "zombie children" problem that react-redux has to deal with (cc @markerikson @dai-shi). A nice explanation is here: https://kaihao.dev/posts/Stale-props-and-zombie-children-in-Redux

To me, it seems like if we had a useMutationEffect / useCaptureEffect that executes from parent to children, before useLayoutEffect, that would:

• Make life easier for implementing react-redux with correct subscription ordering
• Make it safer to generalize the "useGetter" or "useSafeEffect" I mention here, without having the potential problems mentioned by @bvaughn when you pass a callback to a child component.

I think it would be a nice addition to the hooks API.

Also note I've encountered other people as well that told me they were using custom hooks like useStateWithGetter, so it seems more and more people are using these patterns to solve the stale closure

Hey, looking at the newly open-sourced Recoil, it seems it's using a pattern a bit similar to the "getter" pattern: useRecoilCallback which permit to get access to current state in a callback. Not sure yet if the returned callback is "stable" though, will have to check.

So maybe the following will make sense?

useEffect(
  useRecoilCallback(({getPromise}) => {
    const state = await getPromise(stateAtom);
    console.log('state ', state);
  }),
  [when, i, want, to, reexecute]
)

If anyone finds it useful, I've been using this, and it seems to be working well:

export default function useCallback(fn) {
  const ref = React.useRef(fn);
  ref.current = fn;
  return React.useMemo(() => (...args) => ref.current.apply(null, args), []);
}

I put it on NPM at https://www.npmjs.com/package/use-callback

I had issues with attaching the handler during useLayoutEffect, because callbacks were being executed between render but before the useLayoutEffect.

@aantthony note that your example there isn't Concurrent Mode safe, because it's mutating the ref in the middle of rendering.

@aantthony note that your example there isn't Concurrent Mode safe, because it's mutating the ref in the middle of rendering.

@markerikson that‘s interesting, could you explain a bit more why this is a problem in concurrent mode? We mutate refs in our components too, and I would like to make it concurrent mode ready.

@fabb Mutating refs during render can cause bugs, like the one I describe here #18003 (comment)

It would be expected to cause bugs in simple scenarios too though:

• React gets higher priority work and throws away an in-progress render. Your ref is now broken.
• An error happens and React re-renders at the error boundary. Your ref below is now broken.

Wow, I didn’t know this. When is it safe to update a ref then? Just in an event handler?

You can update a ref from inside of useEffect or useLayoutEffect. This is probably the most common way to write to them.

You can also use them for lazy initialization but be careful not to expand this pattern to an update. It's only safe to do if you lazily initialize a value- and even then, only if that value does not read from some other mutable source. It's important that it's idempotent.

Can you elaborate on the use case of updating a ref from within an event handler? I guess you could do that but I'm not sure I understand why you would.

Can you elaborate on the use case of updating a ref from within an event handler? I guess you could do that but I'm not sure I understand why you would.

For example, implementing gestures - coordinating multiple event listeners using refs.

My gut tells me what you would often useState (or useReducer) for that sort of use case, but admittedly I haven't built any gesture recognition stuff. @trueadm is kind of our resident expert in that regard.

@Andarist You should be able to co-ordinate event handlers in effects, rather than in the render phase. That's what we've done internally and also I believe it's how @necolas built React Native Web's responder event system with refs in user-land.

I don't think he was talking about coordinating handlers during the render phase, @trueadm. He was specifically talking about writing to a ref from within an event handler.

He was specifically talking about writing to a ref from within an event handler.

Ah my bad!

In terms of writing to a ref in an event handler, that's perfectly fine to do in this case IMO. That's core to how we've built some our internal event systems at Facebook; for where you have some state that is not related to the render flow, but rather related to the co-ordination of events through event propagation or through streams (multiple event normalization/sequences).

@bvaughn I’ve just spent a month figuring out why ag-grid was leaking memory.

Background:

I found some code was passing api references to useCallback via deps. The apis are mutable, circular and attached as a property to the div element (mounted in their component). That same component calls “destroy” on that api when it is unmounted, but i don’t see any direct code removing that api reference from the DOM element. I originally called a setter on a context provider to set references to their api (for sharing across components), along with commonly used utility methods for stuff like resetting scroll, etc.

Assumptions (Leak Culprits):

• stale closures from useCallback with references to the apis.
• mutable references to the apis on a top level component via context provider.
• no way to destroy/cleanup references at context level, besides useUnmount; their destroy/cleanup getting executed via componentWillUnmount; they do not provide an onDestoy callback.

Solution?

• moved all calculated and context state to useReducer
• Used pure callbacks for dispatching actions to the reducer.
• moved all api references used to calculate state to local useEventCallbacks (ref callback pattern)... calling said setters via actions.
• used a hook in sibling components to access the api via useRef/DOM element prop; making sure to set reference to null on unmount.

Questions:

• am I doing anything unnecessarily?
• is my assumption about useEventCallback for those local callbacks correct?
• Would useCallback suffice? I’m pretty confident the way they are using callback props in their react component is mutating those references as well (plus circular).
• is using a dom property reference in a hook safe? Would you need to set that reference to null when unmounting?
• When handling local state containing their apis, should I useRef because of the mutability?
• Does useState/useReducer both have the same process/pattern as useRef in terms of destroying references when unmounting?
• is manual cleanup necessary?

Sorry @mainfraame. I don't think I have enough context to answer these questions without spending a good bit of time digging into ag-grid to figure out what you're talking about (and I don't have time to do that at the moment).

Hi,

Just wanted to know if there's any news of a potential useMutationEffect or something similar that runs effects top->bottom?

I've seen the recent discussions between @Andarist and @sebmarkbage here: reactwg/react-18#110 (comment) about useInsertionEffect, and wonder if this new effect could help implement useDynamicCallback in a more reliable way (it doesn't look like it's this hook's purpose though).

Curious if there's anything new to avoid the stale closure problems planned for React 18

Hah, when I was writing this comment of mine I was thinking about this issue here and you, Sébastien 😜 According to my understanding - if the order would be flipped to preserve the current injection order of CSS-in-JS libraries then this would solve the problem outlined in the issue here:

function useLatestValueRef(value) {
  const ref = React.useRef(value)
  React.useInsertionEffect(() => (ref.current = value))
  return ref
}

This ones is mostly intended for DOM mutations. The other use case we had in mind for useInsertionEffect is to insert Portal wrapper nodes into a global place. It's nicer if the timing of that is when the children of that portal is already done. We tend to insert at the root once all the children are done. Similarly, DOM nodes are updated after their children so that things like selectedValue works.

We've submitted a proposal to solve this. Would appreciate your input!

reactjs/rfcs#220