facebookarchive / redux-react-hook

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

Question: Passing a store instance to the create function #45

Closed Granipouss closed 5 years ago

Granipouss commented 5 years ago

Hi everyone,

I started experimenting with hooks on my application. I am super happy right now, it's a nice way of simplifying the code and reducing the amount of HOC and wrapper components. But I think we could go event further. On my project, I am using typescript so I use the create function to have typed methods, it's quite handy, but I was wondering if we could maybe pass the store instance to the create method to get rid of the whole provider thing (thus getting rid of another wrapper).

The idea is quite simple, passing a store from the create function to the context as default value, so that we don't need the provider anymore.

// ...

export function create<
  TState,
  TAction extends Action,
  TStore extends Store<TState, TAction>
>(store: TStore | null = null): {
  StoreContext: React.Context<TStore | null>;
  useMappedState: <TResult>(mapState: (state: TState) => TResult) => TResult;
  useDispatch: () => Dispatch<TAction>;
} {
  const StoreContext = createContext<TStore | null>(store);

  // ...

}

Is there any reason why it was not done? Is it a bad idea ?

ianobermiller commented 5 years ago

Yes, I considered that when we added create. However, that would force you to have a global store instance, which is not a best-practice for Redux usage.

Granipouss commented 5 years ago

We could be make the store argument optionnal, so you are not forced to have a global store if you don't want to. It would only add more flexibilty in my opinion. Is having a global store such a bad practice that we want to prevent it ?

ianobermiller commented 5 years ago

Is having a global store such a bad practice that we want to prevent it?

I would say so, yes. As our app has grown we've found several reasons to have multiple stores. It isn't that much work to wrap a provider at the root anyway.

Granipouss commented 5 years ago

When you start having multiple stores, it makes sense indeed that you would do that. I get that wrapping a provider isn't much work, but when you have a provider for the theme, the router, the store, the persistor of the store, etc. I have a feeling that it is adding unnecessary depth to the component tree. This being said, it is most likely not this lib work to solve that issue.

Thanks for the answers and explanations. :)