Skip to content
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

Merged
merged 4 commits into from Aug 1, 2019

Conversation

ryaninvents
Copy link
Contributor

As discussed in issue #1304.

For the create*Hook factory functions, I was debating between using context as the argument to all of them, vs asking users to call createReduxContextHook and pass their custom useReduxContext 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.

@netlify
Copy link

netlify bot commented Jun 11, 2019

Deploy preview for react-redux-docs ready!

Built with commit ae4c0ca

https://deploy-preview-1309--react-redux-docs.netlify.com

src/hooks/useDispatch.js Outdated Show resolved Hide resolved
invariant(selector, `You must pass a selector to useSelectors`)

const { store, subscription: contextSub } = useReduxContext()
function useSelectorWithStoreAndSubscription(
Copy link
Contributor Author

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.

@timdorr
Copy link
Member

timdorr commented Jun 11, 2019

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 useContext factory adds a ton of value. I think it's just as simple as context === ReactReduxContext ? useReduxContext() : useContext(context).

@markerikson
Copy link
Contributor

markerikson commented Jun 11, 2019

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?

@ryaninvents
Copy link
Contributor Author

@markerikson I think that Reddit thread describes what I was originally envisioning, but Tim described the potential issues in this comment.

@timdorr
Copy link
Member

timdorr commented Jun 22, 2019

OK, I think this looks good. Any further comments from anyone else?

@markerikson
Copy link
Contributor

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?

@dai-shi
Copy link
Contributor

dai-shi commented Jun 23, 2019

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 TypeScript
export 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;
};

thought put into just updating the hooks to accept a context object

To me, if we go withcreate*Hook, having createProvider(myContext) seems more consistent. Or, accept a context object for hooks and Provider, which I did so without any thoughtful reason.

@timdorr
Copy link
Member

timdorr commented Jun 23, 2019

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?

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.

@ryaninvents
Copy link
Contributor Author

@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.

@joshjg
Copy link

joshjg commented Jul 16, 2019

@markerikson @timdorr @ryaninvents Anything blocking this from moving forward? Any help needed?

@timdorr
Copy link
Member

timdorr commented Jul 17, 2019

Just updated against master. Provided that passes, I'll merge this in.

@mpeyper
Copy link
Contributor

mpeyper commented Aug 1, 2019

@timdorr just a gentle nudge to come back and merge this.

@timdorr timdorr merged commit 5e6205a into reduxjs:master Aug 1, 2019
@viniciusffj
Copy link

Is this already published on NPM anytime soon?
I can see it in your docs, but it seems to not be available on NPM yet.

@timdorr
Copy link
Member

timdorr commented Aug 5, 2019

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.

@viniciusffj
Copy link

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

@timdorr
Copy link
Member

timdorr commented Aug 26, 2019

This is out as 7.1.1 now!

webguru7 pushed a commit to webguru7/react-redux that referenced this pull request Dec 8, 2022
* feat: implement `create*Hook` APIs

* feat: Hook creators accept context directly

* feat: simplify custom context handling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants