facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.86k stars 46.51k forks source link

Dependency-based reinitialization of useState #14738

Closed jonrimmer closed 5 years ago

jonrimmer commented 5 years ago

Do you want to request a feature or report a bug?

Feature

What is the current behavior?

Consider I want to have a dynamically calculated list of options, and a piece of state that represents the currently selected option. I can achieve this using hooks as follows:

const options = useMemo(() => {
  // Calculate the options
}, [dep1, dep2]);

const [choice, setChoice] = useState(options[0]);

const result = useMemo(() => {
  // Calculate the displayed result
}, [choice]);

However, a problem occurs if either dep1 or dep2 changes. The list of options changes, which means choice may no longer be valid. To fix this, I must split choice into a selected value and a memoized value that checks for validity:

const [selectedChoice, setSelectedChoice] = useState(options[0]);

const choice = useMemo(() => {
  if (options.includes(selectedChoice) {
    return selectedChoice;
  } else {
    setSelectedChoice(options[0]);
    return options[0];
  }
}, [options, selectedChoice]);

What is the expected behavior?

It would useful if we could declare dependencies for useState, in the same way that we can for useMemo, and have the state reset back to the initial state if they change:

const [choice, setChoice] = useState(options[0], [options]);

In order to allow preserving the current value if its valid, React could supply prevState to the initial state factory function, if any exists, e.g.

const [choice, setChoice] = useState(prevState => {
  if (prevState && options.includes(prevState) {
    return prevState;
 else {
    return options[0];
 }
}, [options]);

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

16.8.0-alpha.1

gaearon commented 5 years ago

The idiomatic way to reset state based on props is here:

https://reactjs.org/docs/hooks-faq.html#how-do-i-implement-getderivedstatefromprops

gaearon commented 5 years ago

In other words:

const [selectedChoice, setSelectedChoice] = useState(options[0]);
const [prevOptions, setPrevOptions] = useState(options);

if (options !== prevOptions) {
  setPrevOptions(options);
  setSelectedChoice(options[0]);
}

I don't think we want to encourage this pattern commonly so we're avoiding adding a shorter way (although we considered your suggestion).

WiNloSt commented 5 years ago

I know this is closed but my question is very similar to this. Suppose we want to implement something like getDerivedStateFromProps, but instead of tracking for previousWhatever and manually compare it, use useEffect with that dependency. Would that roughly equivalent for React? I'm not sure the exact rendering order or how this would affect the rendering, because currently in my application, I reset the state when props change in useEffect

For example,

const [selectedChoice, setSelectedChoice] = useState(options[0]);

useEffect(() => {
  setSelectedChoice(options[0]);
}, [options])

Would this work the same as your suggestion above? Is this another idiomatic way of handling getDerivedStateFromProps? Thanks

e1himself commented 4 years ago

Hi :wave:

I've come here with exactly same thinking as @WiNloSt.

I'm building a complex hook that uses state internally. And I want to completely reinitialize it when an incoming parameter is changing. A natural way of doing this would be to provide [deps] array into useState() hook. But there is no such thing :confused:

With the useEffect() approach suggested above I'm afraid there will be unwanted side-effects that may use remembered state values with the new incoming parameter (as we know setState() is asynchronous). So having dependencies list in useState() would really help here.

Thanks.

WiNloSt commented 4 years ago

@e1himself I've been setting state explicitly since the last time I posted here with no issue. To give you more context I'm developing a SaaS app with active users. So I don't think there's gonna be an issue with this approach if you write hook idiomatically (don't lie about the dependencies).

peterjuras commented 4 years ago

@gaearon I understand that this pattern is discouraged, but I think it becomes vital if you want to write hooks and components and that react to updates while being mounted in the correct way. All hooks provide a smooth way to react to updates after being mounted - except useState - since it does not allow for a declarative state reset when it would be needed. While it is possible to work around this issue by setting the state again, it adds more complexity in the code and needs additional render method calls (and while additional render calls shouldn't matter too much in terms of performance, why have them if they can be avoided?)

Current issues with the getDerivedStateFromProps migration documentation

Furthermore, the example of the documentation has some issues, because the hook is returning the wrong value. Let's look at the example:

function ScrollView({row}) {
  const [isScrollingDown, setIsScrollingDown] = useState(false);
  const [prevRow, setPrevRow] = useState(null);

  if (row !== prevRow) {
    // Row changed since last render. Update isScrollingDown.
    setIsScrollingDown(prevRow !== null && row > prevRow);
    setPrevRow(row);
  }

  return `Scrolling down: ${isScrollingDown}`;
}

Here, isScrollingDown is returned which was based on prevRow, although the correct value would be prevRow !== null && row > prevRow. While react will re-render before continuing, the current render method will continue, because the execution is synchronous. This is especially problematic when using hooks and expecting the result to be consistent with its input.

Let's look at a component where transferring the example from the documentation 1 to 1 would lead to issues:

function getAnimationFromType(type) {
  switch (type) {
    case "Scale":
      return { scale: { x: 0, y: 0 } };
    case "Rotate":
      return { rotate: { deg: 0 } };
    default:
      throw new Error("Invalid Type");
  }
}

function useAnimation(type) {
  const [animation, setAnimation] = useState(getAnimationFromType(type));
  const [prevType, setPrevType] = useState(type);

  if (prevType !== type) {
    setAnimation(getAnimationFromType(type));
    setPrevType(type);
  }

  useEffect(() => {
    // TODO: Animate
  }, [animation]);

 return animation; // Warning! This returns an object with properties that don't match the type!
}

function MyComponent({ type }) {
  const animation = useAnimation(type);

  // Let's assume we want to work with a value that has been returned
  // from the hook in the render function. We might receive an Exception, since
  // the returned value from the useAnimation hook might not be in-sync
  // with our type prop.
  let valueFromAnimationHook;
  switch (type) {
    case "Scale":
      // ERROR: This will throw if the type changed, since animation is still based
      // on "Rotate"
      valueFromAnimationHook = animation.scale.x + animation.scale.y;
      break;
    case "Rotate":
      // ERROR: This will throw if the type changed, since animation is still based
      // on "Scale"
      valueFromAnimationHook = animation.rotate.deg;
      break;
    default:
      break;
  }

  return <OtherComponent animation={animation} />;
}

In this example, an exception is thrown when the type changes, since the returned value by the hook is based on a previous prop. This could be fixed by making the state variable re-assignable:

function useAnimation(type) {
  let [animation, setAnimation] = useState(getAnimationFromType(type));
  const [prevType, setPrevType] = useState(type);

  if (prevType !== type) {
    const newAnimation = getAnimationFromType(type);
    setAnimation(newAnimation);
    animation = newAnimation;

    setPrevType(type);
  }

  useEffect(() => {
    // TODO: Animate
  }, [animation]);

 return animation;
}

But it still feels like this adds a lot of complexity to the code, I'm currently even using refs instead of state in a library hook that is used multiple 100 times to ensure that the returned values are consistent and the hook is not responsible for render aborts / re-calls.

How a resettable useState could help

Let's assume that useState has a dependency array argument, similar to other hooks and rewrite our useAnimation hook:

function useAnimation(type) {
  // type is passed as a dependency, if type changes, the current state should be
  // discarded and replaced with the first argument which has been provided as the "initial value".
  // If the type did not change, the state remains untouched and represents the last
  // value that was passed with setAnimation
  const [animation, setAnimation] = useState(getAnimationFromType(type), [type]);

  useEffect(() => {
    // TODO: Animate
  }, [animation]);

 return animation;
}

I see three immediate benefits here:

Conclusion

I really think that a dependency array for useState could add a lot of value. What were the reasons why this hook was the only one that came without it?

e1himself commented 4 years ago

@peterjuras I just realized that it might be possible to achieve similar dependency-based re-initialization effect by using key property mechanics.

<YourStatefulComponent key={id} />

The component will unmount and re-mount every time key property changes.

Though it's a bit hacky and certainly is s less expressive as useState(..., [id]) would be. But at least you'll be able to easily work-around stale state bug described above.

lb215 commented 4 years ago

Stumbled on same issue couple of days ago, I am quite new to React and maybe I had solved this the wrong way, but here is my solution:

import { useState } from 'react'
import { shallowEqualArrays } from "shallow-equal";

function useStateWithDependency(initialValue, dependencyNew) {
    const [{state, dependency}, setState] = useState({state: initialValue, dependencyNew});

    return [(shallowEqualArrays(dependency, dependencyNew)) ? state : initialValue
            , 
            (val) => {
                setState({state: val, dependency: dependencyNew});
            }];
}

export { useStateWithDependency }
mihaisavezi commented 4 years ago

Take this simple example

So I would expect the buttonStatus to change when you fulfill both conditions, but it doesn't happen. I assume this falls under the ops usecase, correct ?

function MyComponent({ type }) {
  const [acceptedTerms, setTerms] = useState(false);
  const [acceptedDataConsent, setConsent] = useState(false);

  const [buttonStatus, setButtonStatus] = useState(
    acceptedTerms && acceptedDataConsent
  );

  return (
    <>
      <Checkbox
        type="checkbox"
        checked={acceptedTerms}
        onChange={() => {
          setTerms(!acceptedTerms);
        }}
      />
      <Checkbox
        type="checkbox"
        checked={acceptedDataConsent}
        onChange={() => {
          setConsent(!acceptedDataConsent);
          debugger;
        }}
      />
      <Button
        style={{ margin: "24px 0" }}
        disabled={!buttonStatus}
        onClick={e => {
          setModalState(true);
        }}
      >
        <Text6>{buttonText}</Text6>
      </Button>
    </>
  );
}
peterjuras commented 4 years ago

Take this simple example

So I would expect the buttonStatus to change when you fulfill both conditions, but it doesn't happen. I assume this falls under the ops usecase, correct ?

function MyComponent({ type }) {
  const [acceptedTerms, setTerms] = useState(false);
  const [acceptedDataConsent, setConsent] = useState(false);

  const [buttonStatus, setButtonStatus] = useState(
    acceptedTerms && acceptedDataConsent
  );

  return (
    <>
      <Checkbox
        type="checkbox"
        checked={acceptedTerms}
        onChange={() => {
          setTerms(!acceptedTerms);
        }}
      />
      <Checkbox
        type="checkbox"
        checked={acceptedDataConsent}
        onChange={() => {
          setConsent(!acceptedDataConsent);
          debugger;
        }}
      />
      <Button
        style={{ margin: "24px 0" }}
        disabled={!buttonStatus}
        onClick={e => {
          setModalState(true);
        }}
      >
        <Text6>{buttonText}</Text6>
      </Button>
    </>
  );
}

Not sure this falls under the same use case, from your code example, wouldn't it suffice to directly calculate the buttonStatus without useState? E.g.:

function MyComponent({ type }) {
  const [acceptedTerms, setTerms] = useState(false);
  const [acceptedDataConsent, setConsent] = useState(false);

  const buttonStatus = acceptedTerms && acceptedDataConsent;

  return (
    // ...
mihaisavezi commented 4 years ago

Not sure this falls under the same use case, from your code example, wouldn't it suffice to directly calculate the buttonStatus without useState? E.g.:

function MyComponent({ type }) {
  const [acceptedTerms, setTerms] = useState(false);
  const [acceptedDataConsent, setConsent] = useState(false);

  const buttonStatus = acceptedTerms && acceptedDataConsent;

  return (
    // ...

indeed that's how I approached it in the end. But I assumed it would work just the same with useState. But oddly it didn't

peterjuras commented 4 years ago

For anyone that runs into this issue, I published a custom hook called useStateWithDeps as an npm package with my implementation proposal from my comment above.

NPM: use-state-with-deps

dhoulb commented 4 years ago

I've needed this a few times. I've made my own workaround helper hook that uses useMemo() (since it does have a deps array).

Regarding the workaround in the Hooks FAQ, there's something I dislike conceptually about the current component render call completing with stale data, then invalidating itself with a setState() to trigger another render. It seems so conceptually scrappy that even though my workaround is less pure I still prefer it.

RumataEstor commented 4 years ago

Agree with @dhoulb. Rendering stale data and then rerunning the renderer again although results with acceptable user-facing outcome but is not only inefficient but is technically incorrect. "We do not encourage this", "you probably don't need this" is not technical enough. You see people face problems, need the feature and the suggested workarounds are inefficient and technically buggy if the renderer uses current prop values along with the state derived off previous props.

Could you please be more technical and describe technical problems to adding dependencies to useState?

guillaume86 commented 3 years ago

So my solution to avoid inconsistent state on the first render and a second render is using useMemo like this:

function useDerivedState<T>(propValue: T) {
  const [stateValue, setStateValue] = useState(propValue);
  const useStateValue = useRef(false);

  // useMemo run synchronously when deps changed
  useMemo(() => {
    useStateValue.current = false;
  }, [propValue]);

  const setValueCallback = useCallback((action: SetStateAction<T>) => {
    useStateValue.current = true;
    setStateValue(action);
  }, []);

  const currentValue = useStateValue.current ? stateValue : propValue;

  return [currentValue, setValueCallback] as const;
}

but the docs specify that useMemo API does NOT garantee that in the future it will run ONLY if deps changed. Is there a safe way to do it or am I stuck with this hack until (and if one day) useState accepts deps?

edit: Nevermind I can store the deps in a ref and manually compare of course...

dhoulb commented 3 years ago

@guillaume86 I noticed that too (after my comment above) and made the same change 😂

Your solution is similar to mine, which has some interesting tweaks you might like:

End result is it creates zero functions or objects on nth render, so performs extremely well. The slowest part is the loop to check if the deps have changed, which I suspect is why useState() doesn't do this itself.

export const useLazyState = <T, D extends readonly unknown[]>(initialiser: T | ((...args: D) => T), deps: D): [T, SetStateAction<T>] => {
    const internals = (useRef<{ value: T; deps: D; setState: SetStateAction<T> }>().current ||= {
        value: initialiser instanceof Function ? initialiser(...deps) : initialiser,
        deps,
        setState: (value: T) => {
            internals.value = value;
            return setState(value);
        },
    });

    const setState = useState<T>(internals.value)[1];

    // Refresh value if the deps change.
    if (!isArrayEqual<D>(deps, internals.deps)) {
        internals.value = initialiser instanceof Function ? initialiser(...deps) : initialiser;
        internals.deps = deps;
    }

    return [internals.value, internals.setState];
};
Venryx commented 2 years ago

@dhoulb Is your solution equivalent to the "use-state-with-deps" solution?

I looked over the source code, and it seems the approach is a bit different (eg. yours uses setState instead of forceUpdate to trigger a call), but it's hard for me to tell which approach is "better".

Is there a particular reason you wrote a custom solution that differs from the use-state-with-deps package? Or is it just a different way of achieving the same thing (eg. to be more compact), without meaningful runtime difference?

dhoulb commented 2 years ago

@Venryx Yep they're equivalent, different only in minor ways. Neither of them are really doing anything that complicated.

The useForceUpdate() in the other solution uses a useReducer() behind the scenes so it's effectively the same anyway (React has a forceUpdate() in its source but we don't have access to it in userland). Personally I think it's more correct to use useState() because it has de-duplication logic, so if the state is updated to the same value it won't refresh the component.