[Feature Request]: distinguish "what" and "when" dependencies in useEffect

[jsamr]

Feature

A new overloading for useEffect (Typescript syntax):

interface useEffect {
  /**
   * @param what - what this side effect does?
   * @param whatDeps - which variables modify “what” the side effect does?
   * These dependencies must match all live variables explicitly referenced
   * in the body of the “what” callback.
   * @param whenDeps - which variables modify “when” the side effect takes place?
   * When and only when at least one of those dependencies change, the “what”
   * callback should be executed.
   */
  (what: (...args: any[]) => any, whatDeps: any[], whenDeps: any[]): void;
  /**
   * @param what - what this side effect does?
   * @param deps - an array of values that the effect depends on.
   *
   */
  (what: (...args: any[]) => any, deps?: any[]): void;
}

Motivations

In the current implementation, the second argument of useEffect, “deps”, is described as such:

The array of values that the effect depends on.

This definition does not account for an important nuance between two kind of dependencies:

• what dependencies, those which require the effect callback to be recomputed updated;
• when dependencies, those which require the effect callback to be rerun executed.

The community seems to be in need of a solution, see https://stackoverflow.com/q/55724642/2779871.

Use case

I want to scroll to top of a component when the content changes (first dependency), but this effect also depends on a variable padding top (second dependency).

With the current implementation of useEffect, this is what I would do:

function MyComponent(props) {
  const { paddingTop, content } = props;
  const ref = React.useRef();
  React.useEffect(() => {
    // scroll to paddingTop when content changes?
    ref.current.scrollTo(0, paddingTop);
  }, [paddingTop, content]);
 return <div ref={ref}>...</div>
}

There is an undesired behavior: the hook is executed on paddingTop changes. Moreover, content is not, semantically, a dependency of the callback, but rather a dependency of when this side effect should take place. So I could use a ref, store the previous value of paddingTop, and compare the two. But that is cumbersome.

What I would like to do, is express the when this side-effect should take place dependencies declaratively:

function MyComponent(props) {
  const { paddingTop, content } = props;
  const ref = React.useRef();
  React.useEffect(() => {
    // scroll to paddingTop when content changes.
    ref.current.scrollTo(0, paddingTop);
  }, [paddingTop], [content]);
 return <div ref={ref}>...</div>
}

Detailed behavior

My understanding is that this proposal would not be a breaking change and is 100% retrocompatible with current implementation.

One argument

useEffect(what);

The behavior is identical to current implementation. The effect is executed after each render cycle.

Two arguments

useEffect(what, deps);

The behavior is identical to current implementation. The second argument conflates whatDeps and whenDeps.

Empty second argument

useEffect(what, []);

The behavior is identical to current implementation. The callback is executed only once.

Empty third argument

useEffect(what, whatDeps, []);

The callback is executed only once, regardless of the changes in whatDeps.

Three arguments

useEffect(what, whatDeps, whenDeps);

The callback is executed when and only when at least one variable in whenDeps array changes, regardless of the changes in whatDeps.

[vkurchatkin]

Am I missing something, or whatDeps doesn't actually affect anything?

[jsamr]

@vkurchatkin I should do a better job at describing the proposal. whatDeps changes will trigger a recomputation of the callback with up-to-date variables, but the callback won't be re-executed before whenDeps changes. So yes, whatDeps does something!

[bvaughn]

• what dependencies, those which require the effect callback to be recomputed;
• when dependencies, those which require the effect callback to be rerun.

What does it mean to "recompute" a callback?

[bvaughn]

An effect callback is just a JavaScript function that closes over variables in scope.

If I'm understanding you correctly, what you're suggesting seems fragile to me. It would make it possible for the function to be invoked with stale (closed over) values. I guess that wouldn't be a concern since– if the values had changed again, we'd update again before calling. But I feel like the mental model you propose is much harder to think about than the single deps array.

[bvaughn]

A change to built-in hooks APIs is really the sort of proposal that should go through our RFC process:
https://github.com/reactjs/rfcs#react-rfcs

[bvaughn]

You could build this hook in user space for what it's worth. At least if I'm understanding you correctly, something like?

function useWhateverYourEffect(callback, whatDeps, whenDeps) {
  const prevWhenValuesRef = useRef([]);

  useEffect(() => {
    // This callback will be called whenever any of the "what" or "when" deps change

    const prevWhenDeps = prevWhenValuesRef.current;
    prevWhenValuesRef.current = whenDeps;

    let changed = false;

    for (let i = 0; i < whenDeps.length; i++) {
      if (prevWhenDeps[i] != whenDeps[i]) {
        // The user callback will be called only when one of the "when" deps change
        callback();
        return;
      }
    }
  }, [...whatDeps, ...whenDeps]);
}

[vkurchatkin]

whatDeps changes will trigger a recomputation of the callback with up-to-date variables, but the callback won't be re-executed before whenDeps changes

What's the point of recomputing the callback if you are not going to run it?

[bvaughn]

I think the main thing being asked for here (if I'm understanding) has nothing to do with "recomputing" really. (I think that's a confusing term to use to refer to a function.)

I think the real request is to only run a callback when certain, specific dependencies change.

[vkurchatkin]

I think the real request is to only run a callback when certain, specific dependencies change.

But that's what useEffect already does

[bvaughn]

Yes and no. It invokes a callback any time any dependency changes. I think @jsamr is saying that he doesn't want the callback to be re-run unless a subset of the dependencies change.

[bvaughn]

I find the proposed API a little confusing. (I think it's easier to reason about a single dependencies array.) But I have heard the question (how to implement this) come up a few times in the past.

[vkurchatkin]

I think @jsamr is saying that he doesn't want the callback to be re-run unless a subset of the dependencies change.

Then you only need to list this subset as dependencies.

[bvaughn]

No, that's not true. You have to specify everything you reference inside of the effect as a dependency– or your callback may close over stale values.

[jsamr]

@bvaughn There must be a better wording for this. For example:

const callback = useCallback(() => {
  //
}, [prop1])

Each time prop1 changes, the callback is "recomputed" (its reference gets updated, obviously I need to change the description).

A change to built-in hooks APIs is really the sort of proposal that should go through our RFC process:

Yes, I'm well aware! But I wanted some help with the phrasing which obviously was required, and basic feedback on the feature itself.

I think the real request is to only run a callback when certain, specific dependencies change.

Absolutely. Any dependency that is not referenced in the body of the callback is, per my definitions, a "whenDep".

[bvaughn]

Absolutely. Any dependency that is not referenced in the body of the callback is, per my definitions, a "whenDep".

I don't think that's true, necessarily. I think more commonly the "when" deps would be referenced too, no?

[vkurchatkin]

You have to specify anything you want to reference inside of the effect as a dependency– or your callback may close over stale values

Well, that's just what linter wants) In practice you list dependencies that trigger effect. Most of the time they match all used values, but sometimes you have to omit something.

[jsamr]

I don't think that's true, necessarily. I think more commonly the "when" deps would be referenced too, no?

A implies B doesn't mean B implies A! So yes, often the "when" deps and "what" deps coincide.

[vkurchatkin]

No. The lint rule is very important and should not be ignored.

Well, I disagree here (I guess I'm "I know what I am doing" kind of person). But, most importantly, this proposed change doesn't actually work around this problem in any way. It just disables linter by adding an unchecked set of dependencies.

[vkurchatkin]

@jsamr I think that's what you want:

function useEffect(fn, whatDeps, whenDeps) {
  const cb = React.useMemo(() => fn, whatDeps);
  
  React.useEffect(() => {
    cb();
  }, whenDeps);
} 

Not sure what's the use case for this is

[bvaughn]

The lint rule is very important.

Omitting values might currently work the way you want– in that they can change without your function being called. That being said, I think there are two problems with this:

1. I think in many cases, when you drill down into a use case, you actually do want your callback to be called (even if you think you don't). Might be useful to talk about some specific use cases though.

2. We have plans (maybe "plans" is too strong a word– "hopes for"? "ideas about"?) a compiler down the road that could automate the dependencies array parts and do component inlining in some cases to make the runtime Fiber tree smaller. Apps written to abuse or ignore the "rules of hooks" (encoded in the lint rule) would not be compatible with such a compiler, since it would result in different behavior.

[jsamr]

@bvaughn Thank you for the implementation you proposed!

I think in many cases, when you drill down into a use case, you actually do want your callback to be called (even if you think you don't). Might be useful to talk about some specific use cases though.

I believe this is not an accurate assumption. If you look at the use case I provided, it depicts a situation where I don't want the scroll effect to be triggered on padding changes, only when content changes. This is very UX-specific.

[bvaughn]

@jsamr I think that's what you want:

I don't think the use of useMemo in that code snippet is really serving a purpose.

I believe this is not an accurate assumption.

It's not so much an assumption as it is based on personal experience from talking about use cases and questions like this. 😄 I didn't say all cases, but I do think many of them.

[vkurchatkin]

The problem with the lint rule is that it works only in subset of cases. Specifically, it implies that effect is idempotent and it "synchronises" some values with some external state, so it's ok and actually good to run as often as possible

[vkurchatkin]

I don't think the use of useMemo in that code snippet is really serving a purpose.

I agree, but it just seems what @jsamr is describing. If you remove it you'll just get plain old useEffect, that is exactly my point.

[bvaughn]

that is exactly my point.

Which I already responded to above. I don't think adding an extra useMemo in does anything but obfuscate things 😅

[bvaughn]

Have to step away from GitHub for a while now.

I suggest moving this conversation to an RFC and closing this issue though!

[jsamr]

@bvaughn I agree, thanks for your invaluable feedback! Do you recommend that I read some relevant references prior to writing the RFC?

[EECOLOR]

@jsamr You can get the desired behavior like this:

function MyComponent(props) {
  const { paddingTop, content } = props;

  const ref = React.useRef();
  const paddingTopRef = React.useRef(null);
  paddingTopRef.current = paddingTop

  React.useEffect(() => {
    // scroll to paddingTop when content changes.
    ref.current.scrollTo(0,  paddingTopRef.current);
  }, [content]);
 return <div ref={ref}>...</div>
}

In my opinion this communicates clearly that, when content changes, you want to set the scroll to the current value of paddingTop. It now is very clear that the effect will not be re-installed when paddingTop changes.

For the record: I would vote against your feature request. I think it adds unneeded complexity to the API for a situation that I have seen very rarely in real world applications.

[jsamr]

@EECOLOR You are absolutely right, thank you. Closing now! See #19820 (comment)

[vkurchatkin]

@jsamr You can get the desired behavior like this:

That is incorrect, specifically this line:

  paddingTopRef.current = paddingTop

You are not allowed to mutate refs during render

[jsamr]

@vkramskikh Well I tried experimentally his solution and it worked. My understanding is that a ref will always be the same throughout all render cycles (hence its name...), so you can do whatever you want with its own attributes; but there might be implications that I am not aware of. Could you elaborate on why this is allegedly "not allowed", or provide a reference in the docs or expert blog? I am surprised that the rules of hooks eslint plugin wouldn't catch this one, if indeed it is considered by React core contributors misuse of the hook API.

[vkurchatkin]

@jsamr

https://reactjs.org/docs/hooks-faq.html#is-there-something-like-instance-variables

Conceptually, you can think of refs as similar to instance variables in a class. Unless you’re doing lazy initialization, avoid setting refs during rendering — this can lead to surprising behavior. Instead, typically you want to modify refs in event handlers and effects.

The language is pretty mild though. It's not a recommendation, more of fundamental rule of React: no side effects in render. Mutating a ref is a side-effect like any other.

I am surprised that the rules of hooks eslint plugin wouldn't catch this one

Looks like there is a PR for this: #19095

[jsamr]

@vkramskikh Thanks for this fair point, I'll reopen.

[jsamr]

I think it adds unneeded complexity to the API

@EECOLOR Given Vladimir insight, I am going to disagree here. The implementation (reworked from @bvaughn proposition) is 8 lines of code:

function useEffectWhen(callback, whatDeps, whenDeps) {
  const prevWhenValuesRef = useRef([]);
  useEffect(() => {
    // This callback will be called whenever any of the "what" or "when" deps change
    const prevWhenDeps = prevWhenValuesRef.current;
    prevWhenValuesRef.current = whenDeps;
    let changed = prevWhenDeps.length !== whenDeps.length || prevWhenDeps.some((d, i) => whenDeps[i] !== d);
    changed && callback();
  }, [...whatDeps, ...whenDeps]);
}

The drawback of a user land implementation is that the linter rules of hooks won't be able to properly track the dependencies.

@bvaughn

It's not so much an assumption as it is based on personal experience from talking about use cases and questions like this. I didn't say all cases, but I do think many of them.

Certainly, I can't speak to that, statistic wise, and you have obviously an order of magnitude more experience than I have. However, even if it takes 1 or 5% of use-cases, I believe it is worth having an official, documented way to do it, supported by the linter rules. I personally find the ref comparison solution very cumbersome.

[vkurchatkin]

The drawback of a user land implementation is that the linter rules of hooks won't be able to properly track the dependencies.

Linter rule can't track this anyway. Linter rule will be able to check whatProps, but whatProps don't actually affect anything, so what's the point?

Also, this:

  const prevWhenValuesRef = useRef([]);
  useEffect(() => {
    // This callback will be called whenever any of the "what" or "when" deps change
    const prevWhenDeps = prevWhenValuesRef.current;
    prevWhenValuesRef.current = whenDeps;
    let changed = prevWhenDeps.length !== whenDeps.length || prevWhenDeps.some((d, i) => whenDeps[i] !== d);
    changed && callback();
  }, [...whatDeps, ...whenDeps])

is doing exactly the same as this:

  useEffect(() => {
    callback();
  }, whenDeps)

So essentially the only thing you get out of this is disabling linter rule, which you can already do

[jsamr]

is doing exactly the same as this:

@vkurchatkin Well, this is not true, because "whatDeps" might have changed, and thus cause the callback to update with new values, whereas in your example, "callback" might have outdated variables in its scope (the famous stale closures @bvaughn was referring to).

[vkurchatkin]

"callback" might have outdated variables in its scope

No, the callback always has the values captured during last render when it is triggered.

[jsamr]

@vkramskikh What you are saying was actually my intuitive approach to the problem until I met the rules of hooks. I could reproduce your claim here, but it is no proof your claim will always be true. @bvaughn previously stated:

your callback may close over stale values

Given that he is a core React contributor, and with all respect and regard for your willingness to help, I would rather put my trust in his statements!

[vkurchatkin]

until I met the rules of hooks

That's the problem. You can't (and shouldn't) solve linter problems with API

I would rather put my trust in his statements!

This sounds reasonable. The problem is that he talks about a different thing. You can't "capture" variables in an effect without running it. Anyway, can you provide an example where the first snippet and the second snippet produce different results?

[EECOLOR]

@vkurchatkin First of all: thank you for pointing this out. It however is more subtle than you describe.

You are not allowed to mutate refs during render
The language is pretty mild though. It's not a recommendation, more of fundamental rule of React: no side effects in render. Mutating a ref is a side-effect like any other.

This is not actually true (the part about it not being allowed, it is true that mutating a ref is a side effect). Check for example: https://reactjs.org/docs/hooks-faq.html#how-do-i-implement-getderivedstatefromprops. Here the side effect is setState. React is quite complicated when you go into these kinds of details. Check this thread for interesting discussions about some of the internals: #14110

The ESLint rules pull request points to this comment which talks about a specific use case where this could cause a problem. Here the returned callback could potentially end up anywhere in the render tree.

I'm not sure if in the use case of paddingTopRef.current = paddingTop it could actually be a problem (even in concurrent mode).

[jsamr]

@vkramskikh If what you say is true, then I totally agree that the linter should be fixed instead.

can you provide an example where the first snippet and the second snippet produce different results?

I would very much like to do so. But I am not familiar with React codebase, and thus I won't be able to guess the conditions for the stale closure inconsistency to occur. @bvaughn Could you or any core contributor help us understand the detailed reasons why Vladimir's solution could lead to stale closures, and eventually provide an example where this is happening? One could start with the example I provided.

EDIT Update example

[vkurchatkin]

This is not actually true (the part about it not being allowed, it is true that mutating a ref is a side effect)

It is true in general. There are a couple of exceptions, specifically calling setState for your own state and initializing refs lazily.

I'm not sure if in the use case of paddingTopRef.current = paddingTop it could actually be a problem (even in concurrent mode).

Of course it could be. It is exactly same thing. In concurrent mode there is no clear concept which render is last or whether it's even going to be used.

and thus I won't be able to guess the conditions for the stale closure inconsistency to occur

Stale closure problem is not related to React internals really. Here is an example:

const [foo, setFoo] = React.useState([]);

useEffect(() => { // this closure captures "current" value of foo
   setInterval(() => { // also this closure
       console.log(foo); // foo is always going to be the same
       // once foo state changes, it's not going to be reflected here
      // hence you can call these 2 closures stale
   }, 1000)
}, []);

The only solution to this issue is to rerun the effect:

useEffect(() => {
   const interval = setInterval(() => { // this closure is always be stale, eventually we will just discard it
       console.log(foo);
   }, 1000);

  return () => {
    clearInterval(interval);
  };
}, [foo]);

[EECOLOR]

I'm not sure if in the use case of paddingTopRef.current = paddingTop it could actually be a problem (even in concurrent mode).

Of course it could be. It is exactly same thing. In concurrent mode there is no clear concept which render is last or whether it's even going to be used.

@vkurchatkin Can you describe a situation where this example would have unexpected results caused by concurrent mode?

function Test({ paddingTop, content }) {
  const contentRef = React.useRef()

  const paddingTopRef = React.useRef(null)
  paddingTopRef.current = paddingTop

  React.useEffect(
    () => { contentRef.current.scrollTo(0,  paddingTopRef.current) },
    [content]
  )
  return <div ref={contentRef}>...</div>
}

[jsamr]

Stale closure problem is not related to React internals really.

I might have misunderstood your point but, if that were true, it would be very surprising that Dan Abramov himself used the expression "the stale closure pitfalls" when introducing exhaustive deps lint rule!

[EECOLOR]

Stale closure problem is not related to React internals really.

@jsamr I think @vkurchatkin means that this is a property of javascript:

function test(date) {
  doAtInterval(() => { console.log(date) }, 1000)
}

test(Date.now())
setInterval(() => { test(Date.now()) }, 1000)

function doAtInterval(f, interval) {
  if (doAtInterval.interval) return
  doAtInterval.interval = setInterval(f, interval)
}

You could say it is related to React internals, but only because React handles a certain behavior for us. So if we use something else for that same behavior, we get the same problem.

I would conclude that you are both right, depending on how you look at the situation.

[bvaughn]

Catching up on this discussion. 😄

No, that's not true. You have to specify everything you reference inside of the effect as a dependency– or your callback may close over stale values.

Could you or any core contributor help us understand the detailed reasons why Vladimir's solution could lead to stale closures

My comment was more of a broad statement that– if you omit dependencies from any of the deps arrays (e.g. useCallback, useEffect, etc.) it can cause problems.

In the scope of this specific discussion, the effect wouldn't have stale values when run (since it would close over the most recent values at the time it runs) but I still think the idea is not a good one for reasons I mentioned above.

However, even if it takes 1 or 5% of use-cases, I believe it is worth having an official, documented way to do it, supported by the linter rules. I personally find the ref comparison solution very cumbersome.

The official recommended way is to use a ref to track the subset of values/changes you care about.

It looks like this issue has settled enough now that I'm going to close it and suggest that the discussion/proposal gets summarized as an RFC:
https://github.com/reactjs/rfcs#react-rfcs

Thanks everyone!

[gaearon]

I should probably note this proposal was initially filed in the RFCs repo (but as an issue). I suggested re-filing here because it didn't seem like it was ready for an RFC, and I was hoping to find time to respond to why I don't think it works in more detail here. An RFC is fine if the author feels ready to write it but I just want to make sure we don't create an impression that we're avoiding the discussion by kicking the issue between two repositories.

[bvaughn]

Thanks for adding that context, Dan.

I had no idea it had been mentioned in the RFC repo initially. Didn't see a cross link or anything. I imagine there's probably enough info now, after the above discussion, for an RFC to be written though 😄

[jsamr]

@bvaughn @gaearon That is just fine. Yes I'll take the time to write the RFC 🙂 I started reading the issue #14920 and realized there is a lot of material relevant to the discussion!

[jsamr]

I also came to think of an other way to define an API to tackle the underlying issue of "executing the useEffect callback on a subset of dependencies changes", which would imply using named dependencies, and is similar to propsAreEqual argument of React.memo. @bvaughn Alluding to your mental model concern, I also think it is easier to grasp:

/**
 * @param callback - The callback to perform a side-effect.
 * @param namedDeps - An object which values will be used as dependencies.
 * @param shouldExecuteCallback - Optional: a function taking previous and
 * current namedDeps and returning true if the callback should be executed,
 * false otherwise.
 */
function useEffectWhen(
  callback,
  namedDeps,
  shouldExecuteCallback = defaultShouldExecuteCallback
) {
  const prevNamedDepsRef = useRef({});
  useEffect(() => {
    // This callback will be executed whenever shouldExecuteCallback returns true
    const prevNamedDeps = prevNamedDepsRef.current;
    prevNamedDepsRef.current = namedDeps;
    (Object.keys(prevNamedDeps).length !== Object.keys(namedDeps).length ||
      shouldExecuteCallback(prevNamedDeps, namedDeps)) &&
      callback();
  }, Object.values(namedDeps));
}

function defaultShouldExecuteCallback(prevNamedDeps, namedDeps) {
  return Object.keys(prevNamedDeps).some(
    (name) => namedDeps[name] !== prevNamedDeps[name]
  );
}

[vkurchatkin]

@jsamr once again, doesn't solve anything: if shouldExecuteCallback doesn't always return true you may get stale closures

[yelouafi]

@jsamr I think it'd be better to move the reasonning down to an alternative useCallback implementation (and also provide an answer to #14099). This way the solution can be used for callbacks used in any context (DOM events, render callbacks ...) not just useEffect (or useLayoutEffect) ones.

In all cases, the solution must take into account that useEffect can use dependencies indirectly, for example by referring to values returned by a useCallback.

The issue is that callabck dependencies play a double rule:

1. avoid the stale closure problem
2. as a recative mechanism to trigger some output from react into the external world

In some case we want both 1 and 2 (like with render callbacks passed to a child Component).

In other cases we want only 1: we just like to update the values seen by the callback. This is typically the case for callbacks that aren't invoked by React but some external notifier like DOM event handlers (unless the handler's code itself changes). React doens't control when those callbacks will be invoked, so it makes sense to always access latest committed state in this case. The solution usually is to close over a mutable ref (there's an ambiguity for setState here, since functional setState could be applied to state that's possibly different from the committed one).

And finally a third (and the most subtle) kind is the useEffect callbacks which may combine both concerns. This corresponds to your motivating example.

A compiler can probably distinguish between callbacks scheduled by React and those invoked by the external world, but the challenge is

• for (2) did the function code itself actually changed or just the closure environment ?
• for (3) how to distinguish between deps that must invalidate and deps that just update the closure?

A manual implementation could be done in user space as mentionned by @bvaughn but the lint rule seems unable to verify deps that aren't passed as an array literal.