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

Add react-apollo-hooks code generation #1295

Merged
merged 7 commits into from
Feb 21, 2019

Conversation

leonardfactory
Copy link
Contributor

Summary

With this PR, i'd like to add generators for react-apollo-hooks, following official React Hooks release.

Hooks generation is behind a config flag (withHooks) and by default is disabled.

Example code generated:

// ...
// Current HOC
export function AddUserMutationHOC<TProps, TChildProps = any>(
  operationOptions:
    | ReactApollo.OperationOption<
        TProps,
        AddUserMutationMutation,
        AddUserMutationVariables,
        AddUserMutationProps<TChildProps>
      >
    | undefined
) {
  return ReactApollo.graphql<
    TProps,
    AddUserMutationMutation,
    AddUserMutationVariables,
    AddUserMutationProps<TChildProps>
  >(AddUserMutationDocument, operationOptions);
}
// Hooks (*new*)
export function useAddUserMutationMutation(
  baseOptions?: MutationHookOptions<
    AddUserMutationMutation,
    AddUserMutationVariables
  >
) {
  return useApolloMutation<AddUserMutationMutation, AddUserMutationVariables>(
    AddUserMutationDocument,
    baseOptions
  );
}

Motivation

Even if react hooks are not officially supported by react-apollo library, there are many discussions around integrating them. Furthermore, current implementation by @trojanowski seems like a perfect way to start using them right now.

Why not another plugin?

All the typings are shared, providing Hooks is just an addition (like HOCs). Generation is optional due to withHooks flag, so it should play well with current usage.

Let me know if there's something I should change, or if you'd prefer a different approach.

@ardatan
Copy link
Collaborator

ardatan commented Feb 8, 2019

@leonardfactory That's really nice! Thank you @leonardfactory .
@DAB0mB Could you review this?

@ardatan ardatan requested a review from DAB0mB February 8, 2019 21:36
@DAB0mB
Copy link
Contributor

DAB0mB commented Feb 9, 2019

Sure I can review it

Copy link
Contributor

@DAB0mB DAB0mB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add useSubscription() support? You can use my implementation of it. I know this is not officially supported yet, but it's likely to be part of the library and released soon. Not to mention that we use it vastly in our WhatsApp Clone.

I also don't think we should have a double post-fix for for hooks e.g. useAddUserMutationMutation should be useAddUserMutation.

Last thing, why do we need to generate React-Components style queries and mutations if we use hooks? Can't we make this conditional based on withHooks?

@ardatan
Copy link
Collaborator

ardatan commented Feb 10, 2019

@DAB0mB For now, can we publish another package for subscription hooks? What do you think?

@leonardfactory
Copy link
Contributor Author

leonardfactory commented Feb 10, 2019

Can you please add useSubscription() support? You can use my implementation of it. I know this is not officially supported yet, but it's likely to be part of the library and released soon. Not to mention that we use it vastly in our WhatsApp Clone.

I'd like to add them, but as you pointed out, it would require to include a useSubscription function (and not only an import) and it seems to me like too much for a codegen library; I'd like to propose instead a importUseSubscriptionFrom and withSubscriptionHooks config option, something like:

withSubscriptionHooks: true
importUseSubscriptionFrom: './addons/react-apollo-subscriptions'

And the plugin will output:

import { useSubscription } from './addons/react-apollo-subscriptions'
// ...
// Actual `useXXXSubscription`

This way, when useSubscription will be available in react-apollo-hooks we can simply change the defaultValue to be react-apollo-hooks. What do you think?

I also don't think we should have a double post-fix for for hooks e.g. useAddUserMutationMutation should be useAddUserMutation.

Oh you're absolutely right! The bad example comes from a problem in my ops naming. Just to be clear, do you mean that a query IsEmailAvailable { ... } should generate a useIsEmailAvailable hook? I'll add it tomorrow if I got it right

Last thing, why do we need to generate React-Components style queries and mutations if we use hooks? Can't we make this conditional based on withHooks?

I was thinking about users who would like to play with hooks without changing existing codebase. Would you like a noComponents flag like the noHOC one?

Thanks for your review anyway!

@DAB0mB
Copy link
Contributor

DAB0mB commented Feb 11, 2019

@leonardfactory Sounds good. I think instead of noComponents: true or noHOC: true you should use withComponents: false and withHOC: false. Feels more consistent.

@leonardfactory
Copy link
Contributor Author

@DAB0mB It feels more consistent to me too, but we already have a noHOC option:

export interface TypeScriptReactApolloConfig extends TypeScriptCommonConfig {
  noGraphqlTag?: boolean;
  noNamespaces?: boolean;
  noHOC?: boolean; // here
  withHooks?: boolean;
}

We can change it to be withHOC, even if it's technically a "breaking" change, migration should be pretty straightforward.

@DAB0mB
Copy link
Contributor

DAB0mB commented Feb 11, 2019

@leonardfactory No don't change if it already exists then. Do you have an example app that uses your features?

@leonardfactory
Copy link
Contributor Author

leonardfactory commented Feb 11, 2019

Can you please add useSubscription() support?

I added them with the importUseSubscriptionFrom option, however I need to import even the SubscriptionHookOptions<T, TVariables> generic type.
I'm thinking about adding a page to the docs with example implementation, changing your current type from SubscriptionOptions to SubscriptionHookOptions to keep it coherent with react-apollo-hooks naming

Sounds good. I think instead of noComponents: true or noHOC: true you should use withComponents: false and withHOC: false. Feels more consistent.

@DAB0mB I tried to introduce changes without breaking anything, so noHOC and noComponents (since they default to false, so existing configuration will continue to output same things), while new options (like withHooks and withSubscriptionHooks) use the with prefix.

I'd like to uniform them to no, but in this way the code will output undesired code, at least until react-apollo-hooks will be merged. Alternatively we can use noHooks and noSubscriptionHooks, while defaulting them to true, even if this sounds weird to me.

My proposal is to keep them as they are, and change them to noHooks when they will be merged in official repo.

Do you have an example app that uses your features?

Not at the moment, just mixed it to something we are developing for a client of ours. Maybe i'll try to set-up something in the next days

👍

@DAB0mB
Copy link
Contributor

DAB0mB commented Feb 12, 2019

@leonardfactory Sounds good. Would like to test it first before I approve it. Overall looks very good and useful

@leonardfactory
Copy link
Contributor Author

@DAB0mB Perfect! Let me know if you find something during your tests. I'll ping you if I get a demo app running.

@DAB0mB
Copy link
Contributor

DAB0mB commented Feb 19, 2019

@leonardfactory I'm testing your modifications with the WhatsApp clone. I use the following config:

noHOC: true
noComponents: true
noNamespaces: true
withHooks: true
withSubscriptionHooks: true
importUseSubscriptionFrom: '../polyfills/react-apollo-hooks'

The only issue I have so far is that it tries to import react-apollo which I don't want to include in my project. Can you fix that?

@DAB0mB
Copy link
Contributor

DAB0mB commented Feb 19, 2019

@leonardfactory
Copy link
Contributor Author

@DAB0mB I disabled both react and react-apollo imports when noComponents option is enabled, seems reasonable?

@DAB0mB
Copy link
Contributor

DAB0mB commented Feb 19, 2019

@leonardfactory Sounds good. Let me do some final check-ups

@leonardfactory
Copy link
Contributor Author

@DAB0mB Ok 👍

@DAB0mB
Copy link
Contributor

DAB0mB commented Feb 20, 2019

@dotansimha @ardatan I approve this PR. It works and looks great to me

@ardatan
Copy link
Collaborator

ardatan commented Feb 20, 2019

I think we should prevent possible errors if ‘unusedImports’ is set in tsconfig.

@leonardfactory
Copy link
Contributor Author

@ardatan do you think using import * as ReactApolloHooks ... would be enough to avoid the warning?

Copy link

@rajington rajington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I can try the branch out locally if there's still concerns.

@ardatan
Copy link
Collaborator

ardatan commented Feb 20, 2019

@leonardfactory Yes! 👍🏻

@leonardfactory
Copy link
Contributor Author

@ardatan Done! 👍 I've added the wildcard imports and tested it, seems fine to me

@ardatan
Copy link
Collaborator

ardatan commented Feb 20, 2019

@leonardfactory Thank you! LGTM @DAB0mB

@DAB0mB
Copy link
Contributor

DAB0mB commented Feb 20, 2019

@leonardfactory Me and the people at The-Guild are very excited about this feature, and we thought of telling the world about it by publishing an article on Meidum. Would you like to submit an article? We can make it under The-Guild's distribution. It would be an honor if so.

@leonardfactory
Copy link
Contributor Author

@leonardfactory Me and the people at The-Guild are very excited about this feature, and we thought of telling the world about it by publishing an article on Meidum. Would you like to submit an article? We can make it under The-Guild's distribution. It would be an honor if so.

Sure, it would be an honour for me! I'll prepare something these days

@rajington
Copy link

Anything I can help with before this is merged? Looking forward to the article!

@leonardfactory
Copy link
Contributor Author

@DAB0mB I've written the draft and I'd like to share it with you before publishing: https://medium.com/@leonardoascione/introducing-hooks-generation-for-react-apollo-2cdc8a7b526d
It has been a while since my last post, so please feel free to tell me if there's something we can do better.
Furthermore, should we publish it after the release so we can link the docs and show version number?

@DAB0mB
Copy link
Contributor

DAB0mB commented Feb 21, 2019

@leonardfactory That's awesome. I'll go through it and give you my thoughts. Let's wait for @dotansimha to merge it and release a new version and then we can see how we can take it further. It's gonna be very useful.

@dotansimha
Copy link
Owner

Thank you @leonardfactory !! It looks GREAT!!!
Merging, and I'll release it as a stable version in a few days, we are currently working on some bug fixes we want to include in the next release.

@dotansimha dotansimha merged commit 5975d30 into dotansimha:master Feb 21, 2019
@leonardfactory leonardfactory deleted the add-react-apollo-hooks branch February 21, 2019 12:05
@trojanowski
Copy link

FYI I just published a new version of react-apollo-hooks (0.4.1) with the useSubscription hook.

@leonardfactory
Copy link
Contributor Author

@trojanowski Awesome! Thank you for notifying, I'll give it a shot this weekend in order to add it here too.

@ardatan
Copy link
Collaborator

ardatan commented Feb 22, 2019

@leonardfactory So we don't need withSubscriptions etc then :)

@leonardfactory
Copy link
Contributor Author

@leonardfactory So we don't need withSubscriptions etc then :)

@ardatan Yes! 🎉

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.

6 participants