Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

useReducer: stale value in closure or lost update? #2913

Open
mischnic opened this issue Jan 5, 2021 · 3 comments · May be fixed by #4580
Open

useReducer: stale value in closure or lost update? #2913

mischnic opened this issue Jan 5, 2021 · 3 comments · May be fixed by #4580
Labels
hooks needs-more-info The issue doesn't contain enough information to be able to help question

Comments

@mischnic
Copy link

mischnic commented Jan 5, 2021

Reproduction

https://codesandbox.io/s/nostalgic-wilson-3vs0s

import React, {
  createElement,
  createContext as createContextOrig,
  useContext as useContextOrig,
  useLayoutEffect,
  useReducer,
  useRef,
  useState,
} from "preact/compat";

function createContext() {
  const context = createContextOrig();

  const ProviderOrig = context.Provider;
  context.Provider = ({ value, children }) => {
    const valueRef = useRef(value);
    const contextValue = useRef();

    if (!contextValue.current) {
      contextValue.current = {
        value: valueRef,
        listener: null,
      };
    }

    useLayoutEffect(() => {
      valueRef.current = value;
      if (contextValue.current.listener) {
        contextValue.current.listener([value]);
      }
    }, [value]);
    return <ProviderOrig value={contextValue.current}>{children}</ProviderOrig>;
  };

  return context;
}

function useContextSelector(context) {
  const contextValue = useContextOrig(context);
  const {
    value: { current: value },
  } = contextValue;
  const [state, dispatch] = useReducer(
    () => {
      return {
        value,
      };
    },
    {
      value,
    }
  );
  useLayoutEffect(() => {
    contextValue.listener = dispatch;
  }, []);
  return state.value;
}

const context = createContext(null);

function Child() {
  const [count, setState] = useContextSelector(context);
  const [c, setC] = useState(0);
  return (
    <div>
      <div>Context count: {count}</div>
      <div>Local count: {c}</div>
      <button
        onClick={() => {
          setC((s) => s + 1);
          setState((s) => s + 1);
        }}
      >
        +1
      </button>
    </div>
  );
}

// Render this
export function App() {
  const [state, setState] = useState(0);
  return (
    <context.Provider value={[state, setState]}>
      <Child />
    </context.Provider>
  );
}

Steps to reproduce

Click on the "+1" button multiple times.

Expected Behavior

What React does: both counters increment simultaneously.

Actual Behavior

With Preact, the first update is ignored and the "local counter" is offset by one to the "global counter".


Came up in dai-shi/use-context-selector#36

cc @marvinhagemeister @dai-shi

@developit
Copy link
Member

developit commented Jan 8, 2021

Curious what the reason is for deferring the assignment of valueRef.current into the layout effect callback - why not just assign it unconditionally in Provider()?

Regarding the stale closure - it seems like the useReducer usage here is incorrect - the reducer function passed to useReducer references the enclosed value binding from the first execution of useContextSelector(), since useReducer's reducer reference is considered immutable. A corrected version would be:

function useContextSelector(context) {
  const contextValue = useContextOrig(context);
  const [state, dispatch] = useReducer(
    () => {
      return {
        value: contextValue.value.current,
      };
    },
    {
      value: contextValue.value.current,
    }
  );
  useLayoutEffect(() => {
    contextValue.listener = dispatch;
  }, []);
  return state.value;
}

@developit developit added hooks needs-more-info The issue doesn't contain enough information to be able to help question labels Jan 8, 2021
@mischnic
Copy link
Author

mischnic commented Jan 8, 2021

I didn't write this code, but apparently it's based on the principle explained here: https://overreacted.io/a-complete-guide-to-useeffect/#why-usereducer-is-the-cheat-mode-of-hooks where local variables are used as well instead of a ref. (dai-shi/use-context-selector#36 (comment))

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Nov 17, 2024

It looks like React defers calling the reducer until the component has rendered which we don't i.e. dispatch will queue up that invocation rather than immediately invoke it like we do in Preact. That seems to be the main difference here, this is similar to the explanation in #2750 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hooks needs-more-info The issue doesn't contain enough information to be able to help question
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants