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

RFC: SchemaExtension #428

Merged
merged 2 commits into from Apr 23, 2018

Conversation

Projects
None yet
3 participants
@leebyron
Copy link
Collaborator

leebyron commented Apr 18, 2018

This adds new grammar to the GraphQL SDL:

extend schema {
  mutation: MutationType
}

This feels like the missing piece of the type system extension framework. In addition to extending types in a type system, the top level schema should be extendable as well.

leebyron added a commit to graphql/graphql-js that referenced this pull request Apr 18, 2018

RFC: SchemaExtension
This adds support for facebook/graphql#428 spec proposal.

So far this just adds language support and updates validation rules to be aware of this new ast node. I'll follow up with support in `extendSchema()` and tests.

@leebyron leebyron referenced this pull request Apr 18, 2018

Merged

RFC: SchemaExtension #1314

RFC: SchemaExtension
This adds new grammar to the GraphQL SDL:

```graphql
extend schema {
  mutation: MutationType
}
```

This feels like the missing piece of the type system extension framework. In addition to extending types in a type system, the top level schema should be extendable as well.

@leebyron leebyron force-pushed the schema-extension branch from a52ecc5 to a86d3d0 Apr 18, 2018

@OlegIlyenko

This comment has been minimized.

Copy link

OlegIlyenko commented Apr 18, 2018

👍 I also got the same feeling after working with other extensions for a while.

On a side note (but I think it is a bit related), I wanted to ask about another limitation that was added at some point:

[RFC] Add Validation rule for unique directives per location.

From the moment this validation was introduced, I felt that it was quite limiting. Considering that now there is a good support for splitting the schema across multiple files, I think that it might be a good idea to reassess this validation.

Do you think it makes sense to reconsider it? (I personally would vote in favor of removing it or, at least, limiting the validation to a subset of directives)

Just to give a small example where it might be helpful. Considering the schema delegation case where multiple GraphQL schemas are merged into one. I can define the config like this:

https://github.com/OlegIlyenko/graphql-gateway/blob/master/testSchema.graphql#L6-L16

With the new extension I can potentially split the schema in 2 files:

# file 1
schema
  @includeGraphQL(
    name: "starWars", 
    url: "http://try.sangria-graphql.org/graphql") {
  query: Query
}

# file 2
extend schema
  @includeGraphQL(
    name: "universe", 
    url: "https://www.universe.com/graphql/beta")

But even if directives are not split across different extensions, I would still find it helpful to be able to use the same directive multiple times (with different arguments) at the same location.

@leebyron

This comment has been minimized.

Copy link
Collaborator Author

leebyron commented Apr 18, 2018

That's certainly something we could reconsider. I remember when applying that rule we did so intentionally to carve out the space for future exploration. Would you mind creating a new issue about that topic where we can discuss further?

@OlegIlyenko

This comment has been minimized.

Copy link

OlegIlyenko commented Apr 18, 2018

Sure thing, I'll create a new issue 👍

@OlegIlyenko

This comment has been minimized.

Copy link

OlegIlyenko commented Apr 19, 2018

leebyron added a commit to graphql/graphql-js that referenced this pull request Apr 19, 2018

RFC: SchemaExtension
This adds support for facebook/graphql#428 spec proposal.

So far this just adds language support and updates validation rules to be aware of this new ast node. I'll follow up with support in `extendSchema()` and tests.
@leebyron

This comment has been minimized.

Copy link
Collaborator Author

leebyron commented Apr 19, 2018

Leaving this open for review for a shorter period of time - I hope this is an easy win :)

@leebyron leebyron merged commit 15b8b40 into master Apr 23, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@leebyron

This comment has been minimized.

Copy link
Collaborator Author

leebyron commented Apr 23, 2018

Thanks for the online and offline feedback everyone!

@leebyron leebyron deleted the schema-extension branch Apr 23, 2018

leebyron added a commit to graphql/graphql-js that referenced this pull request Apr 23, 2018

RFC: SchemaExtension (#1314)
* RFC: SchemaExtension

This adds support for facebook/graphql#428 spec proposal.

So far this just adds language support and updates validation rules to be aware of this new ast node. I'll follow up with support in `extendSchema()` and tests.

* Support extendSchema()

* Formatting edits

* Add parsing and validation tests

* Adjust grammar rules to match spec definitions

@OlegIlyenko OlegIlyenko referenced this pull request Apr 23, 2018

Closed

RFC: SchemaExtension #362

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.