preactjs / preact

⚛️ Fast 3kB React alternative with the same modern API. Components & Virtual DOM.
https://preactjs.com
MIT License
36.85k stars 1.95k forks source link

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

Open mischnic opened 3 years ago

mischnic commented 3 years ago

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 https://github.com/dai-shi/use-context-selector/pull/36

cc @marvinhagemeister @dai-shi

developit commented 3 years ago

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;
}
mischnic commented 3 years ago

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. (https://github.com/dai-shi/use-context-selector/pull/36#issuecomment-754554120)

JoviDeCroock commented 6 days ago

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 https://github.com/preactjs/preact/issues/2750#issuecomment-699101628