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
Merged

RFC: SchemaExtension #428

merged 2 commits into from Apr 23, 2018

Conversation

@leebyron
Copy link
Collaborator

@leebyron 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
This adds support for graphql/graphql-spec#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.
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
Copy link
Contributor

@OlegIlyenko 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
Copy link
Collaborator Author

@leebyron 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
Copy link
Contributor

@OlegIlyenko OlegIlyenko commented Apr 18, 2018

Sure thing, I'll create a new issue 👍

@OlegIlyenko
Copy link
Contributor

@OlegIlyenko OlegIlyenko commented Apr 19, 2018

leebyron added a commit to graphql/graphql-js that referenced this pull request Apr 19, 2018
This adds support for graphql/graphql-spec#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
Copy link
Collaborator Author

@leebyron 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
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
Copy link
Collaborator Author

@leebyron 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

This adds support for graphql/graphql-spec#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants