-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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*Hook
factory functions, I was debating between usingcontext
as the argument to all of them, vs asking users to callcreateReduxContextHook
and pass their customuseReduxContext
as 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.