graphql-extra icon indicating copy to clipboard operation
graphql-extra copied to clipboard

Handling of Operation types (Query/Mutation)

Open GavinRay97 opened this issue 4 years ago • 13 comments

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())?

GavinRay97 avatar Apr 20 '20 01:04 GavinRay97

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.

  • There's going to be only few, mostly cosmetic/typos breaking changes.
  • I've refactored entire "api" lib with class mixins - not that it matters that much but I think it's going to be few times more performant.
  • I'm fixing few tiny bugs and improving test coverage (there was one in upsert)
  • I'm also adding new methods and full operation definition support - there's going to be documentOperationApi() that will handle operation types the same way it's done for schema

Cheers!

vadistic avatar Apr 20 '20 07:04 vadistic

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

GavinRay97 avatar Apr 20 '20 18:04 GavinRay97

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 avatar Apr 28 '20 22:04 vadistic

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

vadistic avatar Apr 29 '20 11:04 vadistic

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

GavinRay97 avatar Apr 29 '20 18:04 GavinRay97

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

GavinRay97 avatar Apr 29 '20 19:04 GavinRay97

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

  • union types too complex to represt sometimes happen to me when there's generic T extending some complex union (eg. ASTNode) - definitely needs investigating

  • typeMap was deleted - the reason for it being that I wanted to have single source of truth AST - and I've realised typeMap could get outdated if you ie. rename type with .update() - I will add it as getter because it's popular api -

  • regexp was because I got lazy and wanted to have posibility to define crud api eg. only over all type definitions XXXTypeDefinition without writing whole kind enum. I've forgot about InputObject. It's an easy fix.

vadistic avatar Apr 30 '20 07:04 vadistic

@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)
    })


vadistic avatar May 01 '20 10:05 vadistic

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)
    })

GavinRay97 avatar May 01 '20 14:05 GavinRay97

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 :)

vadistic avatar May 04 '20 13:05 vadistic

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!

GavinRay97 avatar May 05 '20 15:05 GavinRay97

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.

vadistic avatar May 08 '20 12:05 vadistic

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.

GavinRay97 avatar May 08 '20 14:05 GavinRay97