facebookarchive / redux-react-hook

React Hook for accessing state and dispatch from a Redux store
MIT License
2.16k stars 103 forks source link

Make useMappedState take a memoization array, #30

Closed nmn closed 5 years ago

nmn commented 5 years ago

It's pretty weird that useMappedState requires the input function to be pre-memoized (or be extracted out of the calling function.

useMappedState should work like the other hooks and should take a second argument of an array to memoize with.

This way, the hook would ignore if the function changes, and will only re-subscribe if any of the values passed in the second argument changes.

So this:

const mapState = useCallback(
    state => ({
      canDelete: state.todos[index].canDelete,
      name: state.todos[index].name,
    }),
    [index],
  );

  // Get data from and subscribe to the store
  const {canDelete, name} = useMappedState(mapState);

would become :

const {canDelete, name} = useMappedState(
 state => ({
    canDelete: state.todos[index].canDelete,
    name: state.todos[index].name,
  }),
  [index],
);

Using function defined outside the function etc would continue to work. Further, if no second argument is provided, we can default to the value of the reference of the function provided and the change would remain backwards compatible.

ianobermiller commented 5 years ago

The downside to this is that the eslint rules from the React team to detect missing dependencies in useCallback won't work by default -- you'll have to declare them yourself.

I'm not sure the sugar is worth the mental overhead:

const [v1, v2] = useMappedState(
  useCallback(
    state => [state[prop1], state[prop2]],
    [prop1, prop2],
  ),
);

vs

const [v1, v2] = useMappedState(
  state => [state[prop1], state[prop2]],
  [prop1, prop2],
);

The second is undoubtedly cleaner, and perhaps a final arg memoization array will (or already is) common with custom hooks. If we want this route, we'd also want to update the lint rule (exhaustive-deps, I believe) to accept a set of custom hooks that should also have exhaustive dependencies. However, this would still require users to explicitly opt into the additional safety, instead of being there by default.

knpwrs commented 5 years ago

I mentioned a few ideas over in #35 about how to deal with this. I think my favorite of the bunch would be the WithDeps convention, though that would be a new convention that would need to be introduced.

In that case this hook would be renamed to useMappedStateWithDeps and then for backwards compatibility / flexibility there could be another hook called useMappedState that simply calls the useMappedStateWithDeps hook with an empty array:

export const useMappedState = (fn) => useMappedStateWithDeps(fn, []);

EDIT: I suppose that would actually be backwards compatible.

ianobermiller commented 5 years ago

@knpwrs The React team doesn't recommend creating custom hooks that take dependencies: https://github.com/facebookincubator/redux-react-hook/pull/31#issuecomment-472723328