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

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

Open
duncanleung opened this issue Apr 1, 2019 · 13 comments

Comments

@duncanleung
Copy link

Issue:

  • The docs for useReducer suggests omitting dispatch from the dependencies list of useEffect and useReducer.

    Note
    React guarantees that dispatch function identity is stable and won't change on re-renders. This is why it's safe to omit from the useEffect or useCallback dependency list.

  • The docs also recommend passing dispatch function from useReducer via context to avoid passing callbacks down a large component tree:

  • eslint-plugin-react-hooks warns if dispatch (that's been passed from context) is not added to useEffect dependencies list.

    This conflicts with the first point above (docs suggesting omitting dispatch from the dependencies list)

    React Hook useCallback has a missing dependency: 'dispatch'. Either include it or remove the dependency array.
    

Is this:

  • Expected behavior after passing dispatch through context - so the docs should have a small amendment for this scenario?
  • Or a bug in eslint-plugin-react-hooks?

Thanks for the help!!

Working Sample:

@rbreahna
Copy link

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

@alexkrolick
Copy link
Collaborator

alexkrolick commented Jun 20, 2019

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
Copy link
Author

duncanleung commented Jun 20, 2019

@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
Copy link

@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.

@waynebloss
Copy link

waynebloss commented Oct 11, 2019

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
Copy link
Collaborator

alexkrolick commented Oct 11, 2019

@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} />;
  }
}

@waynebloss
Copy link

waynebloss commented Oct 11, 2019

@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);
}

@waynebloss
Copy link

@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.

@waynebloss
Copy link

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

@waynebloss
Copy link

waynebloss commented Oct 11, 2019

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, []);
}

@waynebloss
Copy link

waynebloss commented Oct 11, 2019

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
Copy link

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

@trombipeti
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants