Some enhancements for hooks #1347

ardatan · 2019-02-24T11:00:49Z

Remove withSubscriptionHooks
Use the same behaviour that @FredyC added in here but in a different way, I think we can uae same withHooks option to provide custom module for hooks.
Fix missing ReactApollo import if noComponents is true, but noHOC is false.
Change noComponents and noHOC to withComponents and withHOC which look more consistent with withHooks.
Fixed invalid import syntax
gqlImport is more consistent with other import syntaxes in codegen. gqlImport: graphql.macro#gql

Signed-off-by: Arda TANRIKULU <ardatanrikulu@gmail.com>

danielkcz · 2019-02-24T11:12:23Z

docs/plugins/typescript-react-apollo.md

@@ -52,13 +52,13 @@ Or if you prefer:
 Customize from where will be `gql` imported. This is useful if you want to use eg. `graphql.macro` for inlining documents.
 Note that you need to write whole import line, eg. `import { gql } from 'graphql.macro'`.

-#### `noHOC` (default value: `false`)
+#### `withHOC` (default value: `true`)


Feels like unnecessary breaking change just for switching logic around. If anything, it should still support old flags, just deprecate them and convert into these.

I agree with @FredyC . I think it's better to change it back @ardatan :)

danielkcz · 2019-02-24T11:16:30Z

docs/plugins/typescript-react-apollo.md

-#### `withSubscriptionHooks` (default value: `false`)
-
-This will cause the codegen to add React **Hooks** even for _Subscriptions_. 
+You can specify alternative module that is exports `useQuery` `useMutation` and `useSubscription`. This is useful for further abstraction of some common tasks (eg. error handling). Filepath relative to generated file can be also specified. But if you provide only `true`, it will use `react-apollo-hooks` by default.


It doesn't feel right to abuse the boolean flag to also have a string value. I would keep it separate, nothing wrong about.

danielkcz · 2019-02-24T11:17:31Z

packages/plugins/typescript-react-apollo/src/helpers.ts

  const config = options.data.root.config || {};
-  return operationType !== 'subscription' || config.withSubscriptionHooks;
+  return config.gqlImport || "import gql from 'graphql-tag'";


Why not include this in the getImports as well?

danielkcz · 2019-02-24T11:22:40Z

packages/plugins/typescript-react-apollo/src/index.ts

 ): Promise<string> => {
+  config.withHOC = 'withHOC' in config ? config.withHOC : true;


Just a personal preference, probably not a big different

Suggested change

config.withHOC = 'withHOC' in config ? config.withHOC : true;

config.withHOC = config.withHOC !== undefined ? config.withHOC : true;

danielkcz · 2019-02-24T11:23:02Z

packages/plugins/typescript-react-apollo/src/root.handlebars

-{{#if @root.config.withHooks}}
-import * as ReactApolloHooks from 'react-apollo-hooks';
-{{/if}}
+{{{getImports}}}


Got burned by this as well :)

Suggested change

{{{getImports}}}

{{{getImports this}}}

…aphql-code-generator into enhancements-hooks

ardatan · 2019-02-24T15:44:59Z

I also think there is another inconsistency between gqlImport and importHooks; because while importHooks takes the name of the module, gqlImport takes all the import expression.

danielkcz · 2019-02-24T15:55:02Z

Yea well, if you have a better idea... I've explained in #1344

danielkcz · 2019-02-24T15:57:10Z

Btw, that's also why there is a different name gqlImport for a full import an hooksImportFrom to clearly denote a difference. That's also why the withHooks should not be used for supplying the module name. It's confusing when someone looks at it without diving into docs.

ardatan · 2019-02-24T16:10:31Z

We use typeModule#ContextType syntax in mappers, so I think this can be used here. The other thing we should consider noGraphqlTag option as well. If this is set to true with gqlImport, TS compiler can give some errors if you set noUnusedImports in your tsconfig.json.
After my change in this PR, if you set gqlImport to false, codegen will render gql tags without any putting gql tags before document strings. I think using two different configuration fields can make inconsistency. If we use two different options there; the code can get messy.
I think same for withHooks and importHooks stuff; because I think we should keep number of config fields as less as possible.

danielkcz · 2019-02-24T16:19:49Z

Ok, it's an interesting approach, I suppose it will work, but it requires a better documentation as it's not the usual approach :)

The other thing we should consider noGraphqlTag option as well. If this is set to true with gqlImport

Yea, this config option is not even documented, so I doubt someone was using it :) What do you think @dotansimha?

After my change in this PR, if you set gqlImport to false, codegen will render gql tags without any putting gql tags before document strings.

I guess it makes sense, not sure who would actually use that, but 🙈

I think same for withHooks and importHooks stuff; because I think we should keep number of config fields as less as possible.

I don't think it makes a difference to have one or two extra configs. It's a DEV tool after all. It's more about clarity so it's apparent from the first view what each config does. You shouldn't be forced look into docs every time you need to tweak something.

leonardfactory · 2019-02-25T15:06:02Z

I'd cast a vote for different options (like I originally implemented them). More options, with clean docs, allows us to output clear errors if plugin is configured badly. I may understand something like we accepts a list of foos or just a single foo, but switching boolean with string seems not so useful in this case. However, enough bikeshedding for today! 🎉

- Add gqlImport config option - Remove `withSubscriptionHooks` - Fix missing `ReactApollo` import if `noComponents` is true, but `noHOC` is false. - Fixed invalid import syntax - `gqlImport` is more consistent with other import syntaxes in codegen. `gqlImport: graphql.macro#gql`

sandiiarov · 2019-03-02T16:14:08Z

We use typeModule#ContextType syntax in mappers, so I think this can be used here. The other thing we should consider noGraphqlTag option as well. If this is set to true with gqlImport, TS compiler can give some errors if you set noUnusedImports in your tsconfig.json.
After my change in this PR, if you set gqlImport to false, codegen will render gql tags without any putting gql tags before document strings. I think using two different configuration fields can make inconsistency. If we use two different options there; the code can get messy.
I think same for withHooks and importHooks stuff; because I think we should keep number of config fields as less as possible.

@ardatan @FredyC But, what if I want to use gql from graphql-tag.macro?
When I use gqlImport: graphql-tag.macro the generated file has import gql from "graphql-tag.macro"; and also has parsed GraphQL queries.

Example with gqlImport: graphql-tag.macro
This is generated file 👇

import gql from "graphql-tag.macro";
import * as ReactApolloHooks from "react-apollo-hooks";

// ====================================================
// Fragments
// ====================================================

export const WitnessInfoFragmentDoc = {
  kind: "Document",
  definitions: [
    {
      kind: "FragmentDefinition",
      name: { kind: "Name", value: "WitnessInfo" },
      typeCondition: {
        kind: "NamedType",
        name: { kind: "Name", value: "Witness" }
      },
      directives: [],
      selectionSet: {
        kind: "SelectionSet",
        selections: [
          {
            kind: "Field",
            name: { kind: "Name", value: "id" },
            arguments: [],
            directives: []
          },
          {
            kind: "Field",
            name: { kind: "Name", value: "firstName" },
            arguments: [],
            directives: []
          },
          {
            kind: "Field",
            name: { kind: "Name", value: "lastName" },
            arguments: [],
            directives: []
          },
          {
            kind: "Field",
            name: { kind: "Name", value: "croppedImage" },
            arguments: [],
            directives: []
          }
        ]
      }
    }
  ],
  loc: { start: 0, end: 103 }
};

How can I stop parsing the GraphQL queries and use graphql-tag.macro with this PR?

I am trying to understand the logic of this 23894f0#diff-518180923419f5ff9c24c6d9b7d61651R86

return config.gqlImport ? JSON.stringify(gqlTag(doc)) : 'gql`' + doc + '`';

If gqlImport is not false, undefined, etc -> transform gql to AST? But this is exactly an opposite that config tells, isn't?

Probably you should consider showing a warning message if noGraphqlTag is set to true with gqlImport instead of changing that code.

danielkcz · 2019-03-02T17:49:07Z

@sandiiarov Do you realize that's what is the macro about? To inline parsed document so it doesn't need to be done on runtime :)

sandiiarov · 2019-03-02T20:52:36Z

@FredyC Yes, I do. Do you understand what I am trying to ask? Do you understand how JSON.stringify(gqlTag(doc)) is working?

sandiiarov · 2019-03-02T21:08:02Z

When I use gqlImport: graphql-tag.macro I want to get

import gql from "graphql-tag.macro";

export const WitnessInfoFragmentDoc = gql`
  fragment WitnessInfo on Witness {
    id
    firstName
    lastName
  }
`;

but getting this

import gql from "graphql-tag.macro";

export const WitnessInfoFragmentDoc = {
  kind: "Document",
  definitions: [
    {
      kind: "FragmentDefinition",
      name: { kind: "Name", value: "WitnessInfo" },
      typeCondition: {
        kind: "NamedType",
        name: { kind: "Name", value: "Witness" }
      },
      directives: [],
      selectionSet: {
        kind: "SelectionSet",
        selections: [
          {
            kind: "Field",
            name: { kind: "Name", value: "id" },
            arguments: [],
            directives: []
          },
          {
            kind: "Field",
            name: { kind: "Name", value: "firstName" },
            arguments: [],
            directives: []
          },
          {
            kind: "Field",
            name: { kind: "Name", value: "lastName" },
            arguments: [],
            directives: []
          }
        ]
      }
    }
  ],
  loc: { start: 0, end: 88 }
};

@FredyC Does it make sense to you?

danielkcz · 2019-03-02T21:35:40Z

Ah, I see what you mean now. Looks like that @DAB0mB introduced that mistake when merging some of @ardatan changes in 859de36. Originally there was different option (which wasn't really documented).

config.noGraphqlTag ? JSON.stringify(gqlTag(doc)) : 'gql`' + doc + '`';

The logic should be reversed with gqlImport option.

dotansimha · 2019-03-16T08:17:52Z

@FredyC you are right, we fixed it in the refactor :)

ardatan changed the title ~~Some enhancements for hooks~~ WIP:vSome enhancements for hooks Feb 24, 2019

ardatan changed the title ~~WIP:vSome enhancements for hooks~~ WIP: Some enhancements for hooks Feb 24, 2019

Some enhancements for hooks

6c3819a

Signed-off-by: Arda TANRIKULU <ardatanrikulu@gmail.com>

ardatan force-pushed the enhancements-hooks branch from de2fb45 to 6c3819a Compare February 24, 2019 11:04

ardatan requested a review from dotansimha February 24, 2019 11:04

ardatan mentioned this pull request Feb 24, 2019

Add hooksImportFrom option #1343

Merged

ardatan changed the title ~~WIP: Some enhancements for hooks~~ Some enhancements for hooks Feb 24, 2019

Some enhancements for hooks

1f7e084

Signed-off-by: Arda TANRIKULU <ardatanrikulu@gmail.com>

ardatan force-pushed the enhancements-hooks branch from 6c3819a to 1f7e084 Compare February 24, 2019 11:17

danielkcz suggested changes Feb 24, 2019

View reviewed changes

ardatan changed the title ~~Some enhancements for hooks~~ WIP: Some enhancements for hooks Feb 24, 2019

Merge branch 'enhancements-hooks' of https://github.com/dotansimha/gr…

ffa63bf

…aphql-code-generator into enhancements-hooks

ardatan added 2 commits February 24, 2019 10:59

Fix test

9f16278

Remove noGraphqlTag, use gqlImport instead

23894f0

ardatan changed the title ~~WIP: Some enhancements for hooks~~ Some enhancements for hooks Feb 24, 2019

ardatan closed this Feb 26, 2019

ardatan deleted the enhancements-hooks branch February 26, 2019 16:31

sandiiarov mentioned this pull request Mar 14, 2019

Fix behavior of gqlImport option #1435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some enhancements for hooks #1347

Some enhancements for hooks #1347

ardatan commented Feb 24, 2019 •

edited

Loading

danielkcz Feb 24, 2019

dotansimha Feb 24, 2019

danielkcz Feb 24, 2019

danielkcz Feb 24, 2019

danielkcz Feb 24, 2019

danielkcz Feb 24, 2019

ardatan commented Feb 24, 2019

danielkcz commented Feb 24, 2019

danielkcz commented Feb 24, 2019

ardatan commented Feb 24, 2019

danielkcz commented Feb 24, 2019

leonardfactory commented Feb 25, 2019

sandiiarov commented Mar 2, 2019 •

edited

Loading

danielkcz commented Mar 2, 2019

sandiiarov commented Mar 2, 2019

sandiiarov commented Mar 2, 2019 •

edited

Loading

danielkcz commented Mar 2, 2019

dotansimha commented Mar 16, 2019

		): Promise<string> => {
		config.withHOC = 'withHOC' in config ? config.withHOC : true;

	config.withHOC = 'withHOC' in config ? config.withHOC : true;
	config.withHOC = config.withHOC !== undefined ? config.withHOC : true;

Some enhancements for hooks #1347

Some enhancements for hooks #1347

Conversation

ardatan commented Feb 24, 2019 • edited Loading

danielkcz Feb 24, 2019

Choose a reason for hiding this comment

dotansimha Feb 24, 2019

Choose a reason for hiding this comment

danielkcz Feb 24, 2019

Choose a reason for hiding this comment

danielkcz Feb 24, 2019

Choose a reason for hiding this comment

danielkcz Feb 24, 2019

Choose a reason for hiding this comment

danielkcz Feb 24, 2019

Choose a reason for hiding this comment

ardatan commented Feb 24, 2019

danielkcz commented Feb 24, 2019

danielkcz commented Feb 24, 2019

ardatan commented Feb 24, 2019

danielkcz commented Feb 24, 2019

leonardfactory commented Feb 25, 2019

sandiiarov commented Mar 2, 2019 • edited Loading

danielkcz commented Mar 2, 2019

sandiiarov commented Mar 2, 2019

sandiiarov commented Mar 2, 2019 • edited Loading

danielkcz commented Mar 2, 2019

dotansimha commented Mar 16, 2019

ardatan commented Feb 24, 2019 •

edited

Loading

sandiiarov commented Mar 2, 2019 •

edited

Loading

sandiiarov commented Mar 2, 2019 •

edited

Loading