react icon indicating copy to clipboard operation
react copied to clipboard

[Feature Request] Allow dependency length to change in hooks

Open georeith opened this issue 5 years ago • 21 comments

Not allowing dependency lists to change size limits the usefulness of useMemo in this particular use case but I imagine there are other similar use cases.

To be clear I am talking about the error triggered here:

https://github.com/facebook/react/blob/024a764310e64ef38963df16970364b9942a60cd/packages/react-reconciler/src/ReactFiberHooks.js#L322

For instance, in my app I have a bunch of items and the user can select an unlimited amount of them, in another component I want to compute an expensive derived value based on this selection that is relevant only to this component, a good use case for useMemo.

However it is not currently possible to use useMemo and I am forced to compute this derived data outside of this component even though I am only interested in doing so whilst this component is mounted.

I don't understand why a change in dependency list length cannot be assumed to be a change in the dependencies itself?

I believe this can be implemented by changing the above to:

if (prevDeps.length !== nextDeps.length) {
    return false;
}

georeith avatar Mar 05 '20 19:03 georeith

This is seems like a non-problem: just add an array itself as a single dependency.

vkurchatkin avatar Mar 05 '20 20:03 vkurchatkin

@vkurchatkin I can't guarantee the referential equality of the array (it is derived itself, a list of ids mapped against a dictionary). The items within it stay referentially the same unless explcitily modified.

Trying to keep the array referentially equal creates a whole host of cache invalidation issues and becomes very fragile in its implementation.

This is a problem anywhere where useMemo would want to reduce a variable length list.

georeith avatar Mar 05 '20 21:03 georeith

Generally in React you should:

  • use immutable structures
  • use pure functions to transform immutable structures

If you follow these two rules, then useMemo just works.

is derived itself, a list of ids mapped against a dictionary

If it is derived, then you should derive it using useMemo as well

vkurchatkin avatar Mar 06 '20 15:03 vkurchatkin

@vkurchatkin I am following all these rules.

I can't use useMemo to create it because of the issue above.

Consider this:

I have normalized dictionary of objects, they are immutable and I change it by doing shallow clones down to the appropriate level of change.

Elsewhere I store a list of selected IDs that reference the keys of that dictionary.

I map that list into an array of the selected objects. I do not want to useMemo that because the IDs don't change but the items do, therefore due to the immutable nature in which I update the items I would be left with stale copies in that array. To useMemo that effectively I need to pass the variable length list of items in as dependencies again.

I now want to derive some information from those selected items. I need useMemo to have the objects as dependencies. I am unable to because the size of the list of objects changes (the selection) and at the same the objects themselves will update (immutably hence why I can't store a referentially equal subset of them in an array).

In short I am trying to follow the principles but in this case useMemo does not work as I am unable to give it a variable dependency list when the data I am computing changes in size.

I could useMemo and pass in the dictionary as a whole buts its hugely inefficient and causes tons of unnecessary recalculations of that data, that I only need to recompute if the selected items change.

georeith avatar Mar 06 '20 16:03 georeith

I don't understand the reasons behind this technical limitation, it feels weird to me that perhaps the simplest, cleanest, possible use of useMemo isn't supported out of the box:

useMemo ( () => x, x );

fabiospampinato avatar Apr 26 '20 15:04 fabiospampinato

I'd like to see this, too.

As a slightly-contrived example, let's consider the following case:

function formValuesReducer(oldValues, update) {
  return { ...oldValues, [update[0]]: update[1] };
}

const initialFormValues = {
  firstName: 'John',
  lastName: 'Doe',
  city: 'Boise',
  state: 'Idaho'
};

const nameTemplatePieces = ['firstName', 'lastName'];

const MyComponent = (props) => {
  const formValues = useReducer(formValuesReducer, initialFormValues);
  
  const nameValues = useMemo(
    () => nameTemplatePieces.map(templatePieces => formValues[templatePieces]),
    [formValues, nameTemplatePieces]
  );
  
  const fullName = useMemo(
    () => doExpensiveOperation(nameValues),
    [nameValues]
  );
}

In the above example, fullName is recalculated any time that any field in formValues changes. Ideally, though, fullName would only be recalculated when either nameValues.firstName or nameValues.lastName has changed.

Since nameTemplatePieces is a constant with a fixed length, we could guarantee that fullName only gets recalculated when nameValues.firstName or nameValues.lastName change by changing this:

const fullName = useMemo(
    () => doExpensiveOperation(nameValues),
    [nameValues]
  );

to this:

const fullName = useMemo(
    () => doExpensiveOperation(nameValues),
    [...nameValues]
  );

However, if the length of nameTemplatePieces (and therefore nameValues) isn't fixed, then this no longer works, because React throws an error saying that the same number of arguments must always be passed to useMemo()

Eli-Black-Work avatar May 21 '20 09:05 Eli-Black-Work

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

stale[bot] avatar Aug 22 '20 19:08 stale[bot]

Yes I still care about this.

fabiospampinato avatar Aug 22 '20 19:08 fabiospampinato

I agree with OP, I also don't understand why instead of throwing an error prevDeps.length !== nextDeps.length can't just count as a change in the dependencies itself.

I want to be able to memoize an array but spreading over the values inside the dependency would throw an error if the array changes in size.

A temporary solution could be to use JSON.stringify(arr) inside hook's deps, but this will only work for serializable data.

jrmyio avatar Sep 23 '20 09:09 jrmyio

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

stale[bot] avatar Dec 25 '20 13:12 stale[bot]

This issue still affects me. Recoil is an example of a library that attempts to overcome this issue of memorising n length derived data from other derived data.

georeith avatar Dec 25 '20 13:12 georeith

Yeah I've been bitten by this one too. Not a useMemo, but the same limitation and error: I wanted to fire a useEffect whenever the contents of a variable length array changes. The array could contain many different data shapes, and I want to check equality using Object.is() style strict equality, so tried to do this:

let items: T[] = [];
// ^ or [{foo: true}], or [1,2,3], or [true] etc depending on the generics used outside of this component

useEffect(() => {
    console.log("changed");
}, items);

And obviously hit this odd limitation.

My potential approaches seem to be:

  • Use one useEffect per item? Items can change length, so the rules of hooks disallow this, which is fine.
  • Use one useEffect and pass in an array of items? The dependency length error is thrown.
  • Transform the array to get it into a format that useEffect is ok with
    • If the array is homogeneous, elements are uniquely identifiable, and the data shape can be known to the hook, then this can sometimes work i.e. let users: User = [{id: 'foo'}]; and useEffect(() => {}, [users.map(user => user.id).join(',')]), but is not suitable for my case as I can't / shouldn't need to know the shape of the items in advance, and I need to use Object.is() style strict equality for comparison.
    • If the array is fine with being checked for equality deeply then useEffect(() => {}, [JSON.stringify(items)]) might be ok sometimes, but is not suitable for my case as I need to use Object.is() style strict equality for comparison
    • If you're ok with a maximum limit on your items then maybe useEffect(() => {}, Array(10).fill(undefined).map((v,i) => items[i])), but is not suitable for my case as I have no reasonable limit I can impose
    • Roll my own hook that circumvents the limitation

So just had to do the last one. It's for useEffect, but equally applicable to the other hooks. My tests say it's ok but please use it with caution.

import {useEffect, useRef} from 'react';

const equal = (a: unknown[], b: unknown[]): boolean => {
    if(a === b) return true;
    if(a.length !== b.length) return false;
    return a.every((e,i) => Object.is(e,b[i]));
};

export const useEffectVariadic = (create: () => (() => void)|void, deps: unknown[]): void => {
    const prevDeps = useRef<unknown[]>([]);
    const count = useRef<number>(0);

    if(!equal(prevDeps.current, deps)) count.current++;
    prevDeps.current = deps;

    useEffect(create, [count.current]);
};

// usage like normal useEffect

useEffectVariadic(() => {
    console.log("hi");
}, variableLengthArrayOfStuff);

dxinteractive avatar Jan 08 '21 04:01 dxinteractive

is derived itself, a list of ids mapped against a dictionary

If it is derived, then you should derive it using useMemo as well

I don't understand this at all. Going with the example of an API function that I don't control that returns a newly created array of results, could you please elaborate on how I could ensure that a certain value is only recalculated when the length or elements of this array change?

lbfalvy avatar Feb 18 '21 12:02 lbfalvy

I noticed that I can add the length of the array and then the elements and this works fine: useMemo(() => x, [x.length, ...x]); The nice thing about it is that it even works for multiple arrays [x.length, ...x, y.length, ...y] And for arrays of arrays [x.length, ...x.map(y => [y.length, ...y]).flat(1)] And any variation of the above.

I actually think that this strategy is much better than just passing arrays because one might accidentally do this: [...x, ...y] which would treat x=[1,2];y=[3,4] and x=[1];y=[2,3,4] as equal even with length mismatch detection, however this should definitely be mentioned in the docs. Currently useMemo doesn't even show an error on different lengths which is very confusing.

lbfalvy avatar Feb 18 '21 16:02 lbfalvy

I don't know how just adding the length and the elements is supposed to prevent the size of the dependencies from changing.

I was thinking something might this might workaround the issue:

type DependencyList = ReadonlyArray<any>;

function sameDeps(a: DependencyList, b: DependencyList): boolean {
  return a.length === b.length && a.every((value, index) => Object.is(value, b[index]));
}

function useMemoDynamic<T>(factory: () => T, deps: DependencyList): T {
  const ref = useRef<DependencyList>();
  const next = ref.current !== undefined && sameDeps(ref.current, deps) ? ref.current : deps;
  useEffect(() => { ref.current = next });
  return useMemo(factory, [next]);
}

This is a very frustrating problem. There are many reasons why the size of the dependencies should be able to grow and shrink while still conforming to all the hook rules.

steinybot avatar Sep 24 '21 09:09 steinybot

@steinybot the length of the dependency array can change and this should be detected, but that would only allow us to flatten one variable length collection into the dependency array, otherwise, eg. if we spread two arrays into a single dependency array, the arrays may change in a way that the resulting dependency array remains the same. Prepending the length of each variable length collection is a comparatively simple way to avoid this problem altogether such that we never even have to think about it.

edit: the dependency array's length still changes but if we prepend lengths in this way then a difference is guaranteed to occur in the shorter of the old and new arrays and therefore comparing lengths is not necessary.

lbfalvy avatar Oct 03 '21 19:10 lbfalvy

After a bit of consideration I've realised that we should special case variable length arrays because allowing them in any dependency list would lead people to make the mistake I've discussed above and there's no way to detect that and issue warnings. React may consider adding a special hook for this because it cannot be intuitively expressed with the current ones, but I went ahead and created this package @lbfalvy/react-utils because if most people are getting by without it then maybe it shouldn't be taking space in the main React bundle.

The package has other stuff as well but it's tree-shakeable so it shouldn't end up in your bundles.

lbfalvy avatar Oct 09 '21 14:10 lbfalvy

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

stale[bot] avatar Jan 08 '22 23:01 stale[bot]

I believe the OP's suggestion is still the correct way to go.

steinybot avatar Jan 09 '22 17:01 steinybot

useMemo dependency length 发生改变的时候,建议是触发更新。就按字面意思来清楚 dependency 的改变,毕竟长度改变也的确证明依赖项发生了改变

// 同时认为如下的方式 是合理的,他们应该被判定为等价
const x1 = [1]; 
const y1 = [2,3,4];

const x2 = [1, 2]; 
const y2 = [3,4];

[...x1, ...y1] === [...x2,...y2]

另外一个问题 为什么 dev 和 prod. 模式下两者表现不一致? 因为有如下代码

// react-hot-loader
var hookWrapper = function hookWrapper(hook) {
  var wrappedHook = function wrappedHook(cb, deps) {
    if (configuration.reloadHooks && deps) {
      var inputs = [].concat(deps);

      // 重点在于它在结尾统一增加了 函数callback 导致数组判断的时候刚好行为不一致
      // reload hooks which have changed string representation
      if (configuration.reloadHooksOnBodyChange) {
        inputs.push(String(cb));
      }

      if (
      // reload hooks with dependencies
      deps.length > 0 ||
      // reload all hooks of option is set
      configuration.reloadLifeCycleHooks && deps.length === 0) {
        inputs.push(getHotGeneration());
      }

      return hook(cb, inputs);
    }
    return hook(cb, deps);
  };
  wrappedHook.isPatchedByReactHotLoader = true;
  return wrappedHook;
};

因为 inputs.push(String(cb)); 的存在,导致。dependency 的依赖项的比较可能跟线上不一致,例如 input1 = [1, 2] output1 = [1,2, String(callback)] input2 = [1, 2, 3] output2 = [1,2, 3, String(callback)]

按照 useMemo 的 dependency 判断(不进行长度判断) prod : input1 == input2 , dev : output2 != output2;

jxh150535011 avatar Dec 16 '22 03:12 jxh150535011

@georeith @Eli-Black-Work I encountered similar problem and written a package which do exactly what you want: It recalculates for each object reference in the array only once, and it is optimally synchronized with the initial array as if you were called arra.map(). Maybe you will want some additional features to it, then you can open an issue or pull request. Or even you can just use its source, i published it under MIT license. Its called use-memo-mapped. Here is the example:

const fullName = useMemoMapped(
    nameValues, // just pass your deps as input array through which you need to map.
    (item, index, originalArray) => doExpensiveOperation(item, index, originalArray) // this will run for each reference only once.
  );

This works only for reference value arrays, not primitives.

AndoGhevian avatar Nov 25 '24 14:11 AndoGhevian

I can't agree MORE with OP. Just consider length change as a normal change, this might be useful sometimes.

NotYoojun avatar Dec 29 '24 08:12 NotYoojun

I also agree with many people above, and would love to know the technical reason why this simple change from OP cannot be done.

I have seen many people use JSON.stringify() as an alternative, but I do not believe strings are guaranteed to be interned in JS unless they are literals. I also believe that under the hood useMemo() uses Object.is() which would make comparing non-interned strings problematic as two identical strings can have different memory locations. This is a nice subtle bug that would not present until strings become long enough, or there is an update to a JS engine.

I like others have written a custom hook to do this:

export function useStableArray<T extends any[]>(unstable: T): T {
  const lastMemoizedArrayRef = useRef<T>(unstable);

  const arraysAreEqual = (a: T, b: T) => {
    if (a.length !== b.length) return false;

    for (let i = 0; i < a.length; i++) {
      if (!Object.is(a[i], b[i])) return false;
    }

    return true;
  };

  return useMemo(() => {
    if (arraysAreEqual(unstable, lastMemoizedArrayRef.current)) {
      return lastMemoizedArrayRef.current;
    }

    lastMemoizedArrayRef.current = unstable;
    return unstable;
  }, [unstable]);
}

ravenscar avatar Feb 28 '25 04:02 ravenscar

I also agree with many people above, and would love to know the technical reason why this simple change from OP cannot be done.

I have seen many people use JSON.stringify() as an alternative, but I do not believe strings are guaranteed to be interned in JS unless they are literals. I also believe that under the hood useMemo() uses Object.is() which would make comparing non-interned strings problematic as two identical strings can have different memory locations. This is a nice subtle bug that would not present until strings become long enough, or there is an update to a JS engine.

I like others have written a custom hook to do this:

export function useStableArray<T extends any[]>(unstable: T): T {
  const lastMemoizedArrayRef = useRef<T>(unstable);

  const arraysAreEqual = (a: T, b: T) => {
    if (a.length !== b.length) return false;

    for (let i = 0; i < a.length; i++) {
      if (!Object.is(a[i], b[i])) return false;
    }

    return true;
  };

  return useMemo(() => {
    if (arraysAreEqual(unstable, lastMemoizedArrayRef.current)) {
      return lastMemoizedArrayRef.current;
    }

    lastMemoizedArrayRef.current = unstable;
    return unstable;
  }, [unstable]);
}

FYI, this hook is not safe. Setting a ref is an effect. The useMemo callback must be pure.

steinybot avatar Feb 28 '25 06:02 steinybot

FYI, this hook is not safe. Setting a ref is an effect. The useMemo callback must be pure. @steinybot

I’m curious, do you know of anything that could actually go wrong here? I get that it should likely be "conceptually pure", but I don’t see how this would fail (due to the impurity, not because of an unrelated bug) in practice.

FlorianWendelborn avatar Aug 09 '25 03:08 FlorianWendelborn