vadistic / graphql-extra

GraphQL AST/SDL toolkit extending graphql/graphql-js with extra features for code generation & testing
https://graphql-extra.netlify.com/globals
MIT License
15 stars 3 forks source link

Handling of Operation types (Query/Mutation) #1

Open GavinRay97 opened 4 years ago

GavinRay97 commented 4 years ago

First of all, I cannot thank you enough for this library. I have built a very large tool for codegen and working with type-definition schemas around it, and it is the best and most comprehensive library for AST manipulation available.

The docs in the readme do not describe much of what you can do, but I found through going the source code and the generated Typescript API docs what it is capable of =D

One thing it does not handle though is queries/mutations, for example:

mutation MyMutation($email: String!) {
  insert_user(objects: { email: $email }) {
    returning {
      id
      email
      name
    }
  }
}

Calling documentApi.addSDL() with this throws:

invalid definition: at ​​​Object.addSDL​​​ ​./my-new-codegen/node_modules/graphql-extra/src/api/document/document.ts:276​

Is there any plan to allow the ability to use an API class with queries/mutations, so that you can do something similar to myDoc.getFields() (IE myQuery.getVariables(), myMutation.getSelectionSet())?

vadistic commented 4 years ago

Glad you like it:)

Yes, it'll land in few days - in let's say 0.3.0 release - I'm like 2/3 done with it.

Cheers!

GavinRay97 commented 4 years ago

That's huge! If it was possible to query Mutation/Query types on the document and parse them as standalone inputs then that is pretty much everything =D

vadistic commented 4 years ago

Ok, added it :) https://github.com/vadistic/graphql-extra/commit/2b22d7334f0457c60b27fd18ce2e3b949054f9fe

I'll take a second look tommorow, add tests and few details but generally it's done :)

Performance wise I'm now afraid to benchmark it - because somehow src with tests grew to over 9k (literally) LOC but feature wise it's neat^^

vadistic commented 4 years ago

@GavinRay97 Ok - I've published it under alpha tag - please let me know how much I've broke you code :upside_down_face:

GavinRay97 commented 4 years ago

Amazing, will do ASAP. We're primarily using the documentApi and fieldApi/inputValueApi, but I'm pumped to peek at new features.

GavinRay97 commented 4 years ago

I've never seen this error before, about the union type being too complex to represent:

image

I assume typeMap was moved to another API name, that I can probably figure out.

Edit: Okay, it's getAllTypes(). One small bug with .getAllObjectTypes(), it doesn't handle input types:

image

Looks like it's counting InputObjectType as an ObjectType, which breaks validation since the nodes are technically distinct:

https://github.com/vadistic/graphql-extra/blob/79573222795cd73bcb75f69a703a28935eee03ff/src/document/document.ts#L209-L211

I think it's because InputObjectTypeDefinition contains ObjectTypeDefinition as a substring so this RegExp is matching it maybe?

https://github.com/vadistic/graphql-extra/blob/2b22d7334f0457c60b27fd18ce2e3b949054f9fe/src/utils/crud.ts#L73-L78

Edit: Changing that to the following makes it work (but maybe it breaks other things, and it was a RegExp instead of a string comparison for a reason?):

return this.arr.filter((node) => node.kind == this.config.kind);

image

vadistic commented 4 years ago

@GavinRay97 Thanks for feedback - that's exactly what I need :)

vadistic commented 4 years ago

@GavinRay97 Ok so the last two ones were one minute fixes, you can check the next alpha, but I cannot reproduce complex union types issue.

I saw this error previously when I was having too complex typings for _underscore apis - I would not be that suprised to see it again, but the eg. getFields/getObjectType is externaly typed facade, hmm...

Could you provide me with reproduction so I can pinpoint it? What are the function interfaces? Where those unions are comming from :)

// this is fine
const getActionType = (document: DocumentApi): Typename | Fieldname => ''

const addSmth = (document: DocumentApi) =>
  document.getObjectType(getActionType(document))
    .getFields().forEach((field) => {
      const actionArgType: GQL.ObjectTypeDefinitionNode | Ast.ObjectTypeDefinitionNodeProps = {} as any

      document.createObjectType(actionArgType)
    })
GavinRay97 commented 4 years ago

Hey, thanks for getting back to me! These are the missing definitions there btw:

(It might look a bit strange, this is for Hasura Actions, which take either a single Mutation/Query type along with other related types for input/output, and we do codegen for language-types + boilerplate REST API endpoints):

I think this will probably go away when I refactor the API, so there isn't a mix of things.

type ActionType = 'Mutation' | 'Query'

/**
 * Returns whether the Action is a Query or Mutation
 * Throws error if neither found
 */
const getActionType = (doc: DocumentApi): ActionType => {
  if (doc.hasType('Query')) return 'Query'
  if (doc.hasType('Mutation')) return 'Mutation'
  else throw new Error('Neither Mutation or Query found in Document SDL')
}

/**
 * Takes an argument from Action field in Schema
 * and converts it to an object type + adds to document
 * To codegen the Action's input parameter types later
 */
const makeActionArgType = (
  field: FieldDefinitionApi
): ObjectTypeDefinitionNode =>
  t.objectType({
    name: field.getName() + 'Args',
    fields: field.getArguments().map((arg) => arg.node),
  })

/**
 * Maps through the Mutation fields to grab Action and creates types
 * in the schema document for each of them for codegen
 */
export const addActionArgumentTypesToSchema = (document: DocumentApi) =>
  document
    .getObjectType(getActionType(document))
    .getFields()
    .forEach((field) => {
      const actionArgType = makeActionArgType(field)
      document.createObjectType(actionArgType)
    })
vadistic commented 4 years ago

It's not that strange, and in any case "as a user" you should be able to do any arbitrary weird thing :)

So I cannot reproduce those complex unions types issue. Maybe I've missed some crucial step or it's some combination of typescript version + tsconfig.

Here's what I've tried: https://github.com/vadistic/graphql-extra/blob/complex-unions-issue/test/complex-union-issue.spec.ts

Here's the same stuff on standalone codesandbox: https://codesandbox.io/s/graphql-extra-issues-cq23x

Could you try reproducting this? I do not want to finally publish next version if it can fail so spectacularly :)

GavinRay97 commented 4 years ago

Hey Jakub,

Apologies for the delay, I will attempt to reproduce today.

Edit: It doesn't look like this one was on you at all. After scratching my head about why you seemed to not have any issues, the only thing I could think of was Typescript itself. So I tried toggling my TS versions between 3.8 and 4.0-dev in VS Code:

https://streamable.com/bq9dnh

Really strange, I wonder if it's worth submitting an issue about. Anyways, all good here and apologies for the confusion!

vadistic commented 4 years ago

No problems with delay. As we see this few days refactor will take almost 3 weeks, but we're getting there :)

It is strange, becasue e.g. getName returns just simple string. I do not see where are those unions can be coming from. Maybe it's about mixin class itself, but still weird.

https://github.com/vadistic/graphql-extra/blob/3cd95cd3a68520724c664b956df78364224e1b03/src/mixin/name.ts#L29-L31

Anyway - I will try RC 3.9 just in case, 4.0 dev is out of scope for now.

GavinRay97 commented 4 years ago

The unions are coming from the new Mixin refactor I think, popped up after 0.3 release.

But also the issue isn't actually valid, meaning that on non-4.0-dev TS it doesn't cause a problem. I filed an issue just on Typescript repo: https://github.com/microsoft/TypeScript/issues/38343

Maybe it's a combination of both -- the type-union complexity did increase recently but 4.0-dev has some regression or bug that lowers it's ability to tolerate complex types? So the two together make for errors.

Worst case scenario is that 4.0 is intended behavior and it's actually broken on post-4.0 but I highly doubt that is the case.