search

facebookarchive / redux-react-hook

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

Allowing use of memoization array. #31

Closed nmn closed 5 years ago

nmn commented 5 years ago

As explained in #30

Hooks usually accept a second argument of values to memoize over. In that respect, the useMappedState is a little weird in that it accepts the mapper function to be pre-memoized or defined as a constant outside of the component.

Accepting an argument of values to memoize based on removes the need to do that.

By defaulting to reference to the mapper function, we get existing semantics when no second argument is provided and the change is backwards compatible.

nmn commented 5 years ago

Have not tested fully, but seems to work in the simple case.

ianobermiller commented 5 years ago

I think this would be a nice, non-breaking addition. We'll need to add some tests first, of course, and we'll probably want to update the documentation.

ianobermiller commented 5 years ago

See #9 and #25 for two other PRs that avoid resubscribing on mapState change. On Sat, Feb 9, 2019 at 1:05 AM Naman Goel notifications@github.com wrote:

@nmn commented on this pull request.

In src/index.ts https://github.com/facebookincubator/redux-react-hook/pull/31#discussion_r255292910 :

@@ -87,7 +78,7 @@ export function useMappedState<TState, TResult>( unsubscribe(); }; },

  • [store, mapState],
  • [store, ...memoizationArray],

I was thinking about maybe creating a memorized wrapper function within the hook such that you never need to resubscribe (unless the store changes) and this way it might be possible to ensure the top-down subscription order.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/facebookincubator/redux-react-hook/pull/31#discussion_r255292910, or mute the thread https://github.com/notifications/unsubscribe-auth/AA2zi5K6rSGjcZec30WCCIudKXeLPHAXks5vLo9tgaJpZM4ayFmn .

ianobermiller commented 5 years ago

@nmn Do you still want to get this PR cleaned up to merge?

ianobermiller commented 5 years ago

Alright, I know this is a popular request, but I consulted the React team and they recommended against custom hooks taking a dependency array. From @sebmarkbage:

We discussed this last week and I think we settled on not recommending that custom hooks take deps.

With a shallow compiler (as opposed to the Prepack strategy), we can only rewrite React components to more optimal form if they directly call a Hook in the react package with no indirection. So adding indirection would defeat those optimizations.

It also adds a source of complexity to the whole ecosystem to have the linter be aware of every possible extension. Every app would have to come with a growing whitelist to enable the lint rule for those Hooks. It's also easy to miss since those that are excluded would just not warn. It really needs to be built-in to the type system at that point.

It's also not very composable since every layer that adds more dependencies just grow.

And even with the best form some of your inputs will be memoized in some cases anyway. Might be better to make that a pattern if needed.

I agree strongly with many of these points, particularly linting which I mentioned earlier. Good to hear also some notes about optimization and composability.