reactjs / react.dev

The React documentation website
https://react.dev/
Creative Commons Attribution 4.0 International
11.04k stars 7.53k forks source link

Does `dispatch` function identity change when passed in context? [Re: eslint-plugin-react-hooks] #1889

Open duncanleung opened 5 years ago

duncanleung commented 5 years ago

Issue:

Is this:

Thanks for the help!!

Working Sample:

rbreahna commented 5 years ago

@duncanleung any luck on this? I'm having the same issue

alexkrolick commented 5 years ago

If you consume dispatch from contex (in a different module than the reducer + context provider), the lint rule can't tell if the dispatch comes from useReducer or not. It only works for callbacks/effects defined in the same component as the reducer.

duncanleung commented 5 years ago

@rbreahna I believe I may have manually turned off the eslint check for this one hook

  useEffect(() => {
    dispatch({ type: "add" });
  }, [dispatch]); // eslint-disable-line react-hooks/exhaustive-deps
rbreahna commented 5 years ago

@alexkrolick that's very inconvenient. Many people are building their own redux analog with context and useReducer and consuming dispatch from context in that way. Disabling the eslint rule on every occurrence should not be the standard approach. I hope that this gets a fix or some guidance. Maybe a config to ignore functions of a specific name in .eslintrc.

ghost commented 5 years ago

I came here to ask a similar question or perhaps the same exact question. The following is my experimental code with useReducer that, instead of passing dispatch down to my child components, wraps dispatch in a more convenient function called updateAccount.

This is in my parent component:

  const [accounts, dispatch] = React.useReducer((state, action) => {
    const { type, id, account } = action;
    if (type === "update") {
      return {
        ...state,
        [id]: account,
      };
    }
  }, {});

  const updateAccount = React.useCallback(
    (id, account) => {
      dispatch({
        type: "update",
        id,
        account,
      });
    },
    [dispatch],
  );

Here's how my child component calls updateAccount after some of its local state changes:

    React.useEffect(() => {
      updateAccount(account.id, {
        id: account.id,
        fieldX,
        fieldY,
      });
    }, [account, updateAccount, fieldX, fieldY]);

After creating this code and upon remembering the documentation referenced here - I naively thought "OK, I can remove [dispatch] from my updateAccount function and then if I do that, why not remove React.useCallback altogether because at that point it's not doing anything."

But after trying it out, obviously my child components final React.useEffect call was firing in an infinite loop because updateAccount was changing, causing the parent components reducer state to update, causing the child component to re-render.

So, maybe the docs should remove " or useCallback" from the tip that says "This is why it's safe to omit from the useEffect or useCallback dependency list.". Or, perhaps, they can expand on when you should use it with useCallback.

alexkrolick commented 5 years ago

@waynebloss you can omit dispatch from useCallback dependencies there, but not omit useCallback itself if you want a stable function reference across renders. useCallback and useMemo are basically used in places where you would declare a function like this in class components:

class Foo extends React.Component {
  state = { count: 0 };
  // this function has a stable identity when re-rendering
  handleClick = () => { this.setState(({count}) => ({ count: count + 1 })) };
  render() {
    return <MemoizedButtonOrSomething onClick={this.handleClick} />;
  }
}
ghost commented 5 years ago

@alexkrolick Thanks. That works unless I wrap my call to useReducer into another function that I created called useReducerMap as shown below.

So, I guess that's why we're here on this issue :) The docs definitely need to be clarified or the eslint rule fixed to detect where dispatch originated from.

/**
 * Calls `React.useReducer` with the given action handler mapping, returning
 * state and a dispatch method.
 * @template R
 * @template I
 * @param {{ [action:string]: React.ReducerAction<R> }} handlers
 * @param {(I & React.ReducerState<R>)} [initialState]
 * @param {((arg: I & React.ReducerState<R>) => React.ReducerState<R>)} [initializer]
 */
export function useReducerMap(handlers, initialState, initializer) {
  /**
   * @param {(I & React.ReducerState<R>)} state
   * @param {React.ReducerAction<R>} action
   */
  function reduceWithHandler(state, action) {
    const { type } = action;
    const handler = handlers[type];
    if (typeof handler !== "function") {
      throw new Error("Unknown action type: " + type);
    }
    return handler(state, action);
  }
  return React.useReducer(reduceWithHandler, initialState, initializer);
}
ghost commented 5 years ago

@alexkrolick Actually, it doesn't work because when I do that, it caused the following error:

Maximum update depth exceeded. This can happen when a component calls setState inside useEffect, but useEffect either doesn't have a dependency array, or one of the dependencies changes on every render.

ghost commented 5 years ago

@alexkrolick OK, I omitted the entire array [dispatch] and not just the first element dispatch, so passing an empty array fixed that.

ghost commented 5 years ago

Another solution that will probably work here is to wrap useCallback like I did for useEffect here, which completely defeats the exhaustive-deps rule by hiding the call. It also makes it easier to never run into the bug where you forget to pass an empty array:

/**
 * Hook to run a handler once on mount and never again.
 * @param {() => void} handler Function to run on mount.
 * See https://reactjs.org/docs/hooks-reference.html#useeffect
 * See https://github.com/facebook/create-react-app/issues/6880
 * This function is mainly to provide a better signal to the developer than
 * knowing how `useEffect` works when you pass an empty array. It also helps to
 * get around warnings raised by `react-hooks/exhaustive-deps` and we use it
 * instead of `// eslint-disable-next-line react-hooks/exhaustive-deps`.
 */
export function useOnMount(handler) {
  // Passing an empty array to `useEffect` causes the handler to only run once.
  // See the final API notes for `useEffect` in the docs (link above).
  return React.useEffect(handler, []);
}
ghost commented 5 years ago

Here it is:

/**
 * Hook to create a callback function with `react-hooks/exhaustive-deps` disabled,
 * such as for making a call with `dispatch` from `React.useReducer`.
 * @param {() => void} handler The function that probably uses `dispatch`.
 * See https://reactjs.org/docs/hooks-reference.html#usecallback
 * See https://github.com/reactjs/reactjs.org/issues/1889
 * This function is mainly to provide a better signal to the developer than
 * knowing how `useDispatch` works when you pass an empty array. It also helps
 * get around warnings raised by `react-hooks/exhaustive-deps` and we use it
 * instead of `// eslint-disable-next-line react-hooks/exhaustive-deps`.
 */
export function useFunction(handler) {
  return React.useCallback(handler, []);
}

// Used here:

/**
 * State reducer hook for editing objects by id.
 * @template R
 * @template I
 * @param {{ [action:string]: React.ReducerAction<R> }} [handlers]
 * @param {(I & React.ReducerState<R>)} [initialState]
 * @param {((arg: I & React.ReducerState<R>) => React.ReducerState<R>)} [initializer]
 * @returns {([ I, { [action:string]:(...args:any[])=>any }, React.Dispatch<any> ])}
 */
export function useEditByIdState(handlers, initialState = {}, initializer) {
  // #region Handlers
  const defaultHandlers = {
    update(state, { id, value }) {
      return {
        ...state,
        [id]: value,
      };
    },
    updateAll(state, { values }) {
      return {
        ...state,
        ...values,
      };
    },
    updateField(state, { id, field, value }) {
      return {
        ...state,
        [id]: {
          ...state[id],
          [field]: value,
        },
      };
    },
  };
  if (!handlers) {
    handlers = defaultHandlers;
  } else {
    handlers = {
      ...handlers,
      ...defaultHandlers,
    };
  }
  // #endregion
  const [state, dispatch] = useReducerMap(handlers, initialState, initializer);
  // #region Action Dispatchers
  const actions = {
    update: useFunction((id, value) => dispatch({ type: "update", id, value })),
    updateField: useFunction((id, field, value) =>
      dispatch({ type: "updateField", id, field, value }),
    ),
    updateAll: useFunction(values => dispatch({ type: "updateAll", values })),
  };
  // Could probably use something like `bindActionCreators` from `redux` on these 
  // dispatchers, but let's not go crazy!
  // #endregion
  return [state, actions, dispatch];
}

My new parent component code:

const [accounts, { update: updateAccount }] = useEditByIdState();

Child component code stayed the same.

Perhaps I could name these reducers better than useFunction and useEditByIdState but this is all highly experimental for me and I'll probably just revert to using something like Formik instead because even just lifting state for a simple array of objects editor form was too much work with Hooks since I ran into so many infinite loops while figuring it out and it will probably only serve to confuse junior devs with my limited documentation.

castlewu9 commented 4 years ago

I'd rather add eslint-disable-line. code will be too complicated to keep the eslint rules for this.

trombipeti commented 2 years ago

Hi, any update on this?

I beleive adding eslint-disable-line is worse than just adding the unnecessary dependencies, because in that case you wouldn't get a warning if later you modified the code and used some state value or something, but forgot to add it to the dependency list.

Would it be possible to add an extra parameter to eslint-disable-line (like // eslint-disable-line react-hooks/exhaustive-deps [dispatch, setSomeValueFromCustomHook]? Or maybe be able to list "static" function names in .eslintrc?