nicolasdao / graphql-s2s

Add GraphQL Schema support for type inheritance, generic typing, metadata decoration. Transpile the enriched GraphQL string schema into the standard string schema understood by graphql.js and the Apollo server client.
Other
187 stars 15 forks source link

Support for AWS AppSync Subscriptions #14

Closed cameronrll closed 6 years ago

cameronrll commented 6 years ago

Hi there,

Wondering what the best way to add support for AWS AppSync Subscriptions would be.

The format is the following

type Subscription{
    commentCreated(....): Comment
        @aws_subscribe(mutations:["createComment"])
}

Having looked through the source code I have managed to very quickly fix this issue for myself by doing the following.

graphmetadata.js (original)

const removeGraphMetadata = (schema = '') => {
    const meta = extractGraphMetadata(schema) || []
    const directives = meta.filter(m => m.directive)
    const schemaWithNoMeta = (reinsertDirectives(meta.escSchema.replace(/@(.*?)_cr_/g, ''), directives) || '').replace(/_cr_/g, '\n')
    return { stdSchema: schemaWithNoMeta, metadata: meta }

    //escapeGraphQlSchema(schema, carrReturnEsc, tabEsc).replace(/_t_/g, ' ').replace(/@(.*?)_cr_/g, '').replace(/_cr_/g, '\n')
}

working version with AWS AppSync Subscriptions

const removeGraphMetadata = (schema = '') => {
    const meta = extractGraphMetadata(schema) || []
    const directives = meta.filter(m => m.directive)
        const schemaWithNoMeta = (reinsertDirectives(meta.escSchema, directives) || '').replace(/_cr_/g, '\n')
    return { stdSchema: schemaWithNoMeta, metadata: meta }

    //escapeGraphQlSchema(schema, carrReturnEsc, tabEsc).replace(/_t_/g, ' ').replace(/@(.*?)_cr_/g, '').replace(/_cr_/g, '\n')
}

By removing the replace that takes place on the "schemaWithNoMeta" line I was able to get this to work. I appreciate this is not a nice fix, and very likely introduces breaking changes for others not using AppSync.

Perhaps this replace could take place in a function, that if it detects "@aws_subscribe(...)" it will not remove it?

Happy to open a PR into this and take a better look over the coming days, just wondering if any extra guidance can be provided on what others are using @... for!

nicolasdao commented 6 years ago

Interesting @cameronrll . Let me have a look. I'll come back to you ASAP.

cameronrll commented 6 years ago

Am more than happy to work some more on the PR, tidy it up, and have it approved, if that works for you @nicolasdao ?

nicolasdao commented 6 years ago

Go for the PR @cameronrll . However, because I haven't had time to look deeper into your issue, I'm not really sure what's the real problem with AppSync and GraphQL subscription. Subscriptions are already supported for standard GraphQL API, so I'm not sure why AppSync differ.

Can you enlight me?

cameronrll commented 6 years ago

Yeah sure, seems like I forgot to add that bit to the above! So currently this package will incorrectly strip the "@aws_subscribe" tag from the schema, so after "transpileSchema" is ran, this

type Subscription{
    comments(....): Comment
        @aws_subscribe(mutations:["createComment", "updateComment"])
}

would end up looking like

type Subscription{
    comments(....): Comment
        "createComment"
        "updateComment"
        ])
}

I have submitted a PR which fixes this issue, but retains this stripping behaviour for anything that isn't the "@aws_subscribe directive. Please feel free to offer any improvements or cleanup you would like to see.

https://github.com/nicolasdao/graphql-s2s/pull/16

balciseri commented 6 years ago

I'm running into a similar issue, the resulting shema has removed my custom @directives why are we removing them ? For example i'm using the output to feed a Prisma deploy command but it wont work because graphql-s2s is removing all the @directive that are needed!

Or another example: https://www.apollographql.com/docs/engine/caching.html#hints-to-schema it's impossible to even implement apollo cache system because for ex:@cacheControl(maxAge: 240) is removed.

Am I missing something? Directives are important, if we are removing them we lose infos

nicolasdao commented 6 years ago

Hi @balciseri,

I'm fixing that issue asap. The reason graphql-s2s is removing some directive (currently removing the ones that prepends a field) is because originally we added support for metadata. The convention for metadata is unfortunately similar to directives. Because metadata are not natively supported by GraphQL, graphql-s2s strips them out from the schema, removing directives at the same time.

We're actively looking into this issue.

Cheers,

Nic

nicolasdao commented 6 years ago

Hi @balciseri and @cameronrll,

I've identified the root issue. I added support for directive a while ago as other members of our community experienced the same issue (https://github.com/nicolasdao/graphql-s2s/issues/10). Directives are supported, but the reason why @aws_subscribe is causing issues is that it is not explicitly defined inside the schema (e.g., directive @aws_subscribe on QUERY | FIELD).

I'm looking into that issue now.

Thanks for your patience.

Cheers,

Nic

nicolasdao commented 6 years ago

Hi @cameronrll and @balciseri,

I've just fixed this issue. Install the latest version to see those changes (0.16.4). However, there is a slight catch. This fix only works if you apply a directive on the same line as the field you're decorating. Example:

CORRECT

type Subscription {
    nameCreated: Name @aws_subscribe(mutations:["createName"])
}

INCORRECT

type Subscription {
    nameCreated: Name 
       @aws_subscribe(mutations:["createName"])
}

Let me know if that's good enough for you and if that works properly.

Thanks a lot for your support and for helping the community to make this tool better.

Cheers,

Nic

balciseri commented 6 years ago

@nicolasdao thanks for the fix.

There is still a small thing. @cacheControl(maxAge: 240) can also be next to the type and not only next to the field. https://www.apollographql.com/docs/engine/caching.html#hints-to-schema

type Post @cacheControl(maxAge: 240) {
  id: Int!
  title: String
  author: Author
  votes: Int @cacheControl(maxAge: 500)
  readByCurrentUser: Boolean! @cacheControl(scope: PRIVATE)
}

This version is acting strange if i add the directive next to the type but works if i add it next to the field.

nicolasdao commented 6 years ago

Damn... I missed that use case :)

Thanks for pointing that out @balciseri .

I'll fix that one too.

Thanks a lot for finding that one too.

Cheers,

Nic

nicolasdao commented 6 years ago

Hi @balciseri,

I've just fixed this issue. Install the latest version to see those changes (0.16.5) and this should now work.

Thanks a lot for continually reporting those issues. This is highly appreciated.

Cheers,

Nic

balciseri commented 6 years ago

Great job, thanks! 💯