feat: implement create*Hook APIs#1309
Conversation
|
Deploy preview for react-redux-docs ready! Built with commit ae4c0ca |
| invariant(selector, `You must pass a selector to useSelectors`) | ||
|
|
||
| const { store, subscription: contextSub } = useReduxContext() | ||
| function useSelectorWithStoreAndSubscription( |
There was a problem hiding this comment.
I factored this out to make the diff easier to read, but can inline it again if you'd prefer.
|
So, I was kind of expecting this: export const myContext = React.createContext(null)
export const useStore = createStoreHook(myContext)
export const useDispatch = createDispatchHook(myContext)
export const useSelector = createSelectorHook(myContext)I don't think creating a |
|
As a vaguely related note - @drcmda made some kind of suggestion on Reddit about somehow using closures to avoid needing a Provider or context at all. I don't actually get what he's suggesting there, but maybe he can pop in and explain? |
f1ee17d to
062409f
Compare
062409f to
e9631ae
Compare
|
@markerikson I think that Reddit thread describes what I was originally envisioning, but Tim described the potential issues in this comment. |
|
OK, I think this looks good. Any further comments from anyone else? |
|
Just out of curiosity, was there any discussion or thought put into just updating the hooks to accept a context object either as a direct parameter or in an options object? |
|
If it's still not too late, I'd like to open a discussion to create all hooks and a provider in one function. const { Provider, useStore, useDispatch, useSelector } = createCustomHooks(myContext);This could make type inference easier for DefinitelyTyped and flow-typed. Example in TypeScriptexport type CreateCustomHooks = <SS, A>(myContext: Store<SS, A>) => {
Provider: Provider<A>;
useStore: Store<SS, A>;
useDispatch: () => Dispatch<A>;
useSelector: <TSelected>(
selector: (state: SS) => TSelected,
equalityFn?: (left: TSelected, right: TSelected) => boolean
) => TSelected;
};
To me, if we go with |
I'm much more in favor of composition over configuration. We've already established a 2nd arg on useSelector, so now remembering argument order would come into play. Also, we'll have to handle defaults or do the polymorphic stuff that createStore does. A factory function feels like a better option, especially when it makes reuse easy. |
|
@markerikson I thought about adding an extra parameter, but then all hooks using the default context (which would be the vast majority) would see a very small performance hit. I realize that it's unlikely that we'd even be able to measure the perf hit, but on principle I felt as though this enhancement for the edge case shouldn't impact the vast majority of users who would never run into this problem. @dai-shi There's some related discussion in the feature request issue; I'd originally implemented it the way you describe. |
|
@markerikson @timdorr @ryaninvents Anything blocking this from moving forward? Any help needed? |
|
Just updated against master. Provided that passes, I'll merge this in. |
|
@timdorr just a gentle nudge to come back and merge this. |
|
Is this already published on NPM anytime soon? |
|
Not yet. I need to separately version the master branch in the docs so that it's clear this isn't in the released version. |
|
Yes, it would be great. Yesterday I spent a long time trying to use this feature by following the docs. Let me know if there is anything that I can do to contribute with that split between doc/master |
|
This is out as 7.1.1 now! |
* feat: implement `create*Hook` APIs * feat: Hook creators accept context directly * feat: simplify custom context handling
As discussed in issue #1304.
For the
create*Hookfactory functions, I was debating between usingcontextas the argument to all of them, vs asking users to callcreateReduxContextHookand pass their customuseReduxContextas the argument to the remaining three. I chose the latter since it seemed like less churn, but I'd be happy to explore other options or refactor as needed.