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

SDL Spec changes #1117

Merged
merged 1 commit into from Dec 6, 2017

Conversation

Projects
None yet
4 participants
@leebyron
Copy link
Collaborator

leebyron commented Dec 6, 2017

This adds the recent changes to the SDL proposal.

@IvanGoncharov

This comment has been minimized.

Copy link
Collaborator

IvanGoncharov commented Dec 6, 2017

@leebyron Here is an idea to discuss.
Maybe it would be better to have one TypeExtensionNode instead of:

  • ScalarTypeExtensionNode
  • ObjectTypeExtensionNode
  • InterfaceTypeExtensionNode
  • UnionTypeExtensionNode
  • EnumTypeExtensionNode
  • InputObjectTypeExtensionNode

It could be defined as:

type TypeExtensionNode {
  type: TypeDefinitionNode
}

It became possible since this PR makes fields, values, etc. optional inside type definitions.
Removing ObjectTypeExtensionNode introduces breaking change but it simplifies working with extensions, e.g. to visit all extension you need to register one function instead of six.

@leebyron

This comment has been minimized.

Copy link
Collaborator

leebyron commented Dec 6, 2017

I like that idea and we actually had this arrangement in the definition of ObjectTypeExtensionNode before a previous PR that adopted an earlier wave of spec compliance. I ended up realizing that the idea did not work when realizing that the logic for visiting a definition node and visiting an extension node will almost always be different, so the unification wouldn't aid much for common usage. In that case you would need to track the parent node to fork behavior.

There are also still some subtle differences between the two. Extensions cannot be empty and extensions do not add descriptions. Since the grammar for the two are slightly different I think it makes sense to introduce new AST node types, even if the differences are subtle.

I think if we have the desire to visit all extensions in a single function, that we might instead add functionality to the visitor

@leebyron leebyron force-pushed the sdl-improvements branch from be1787a to c2f42fc Dec 6, 2017

@mjmahone
Copy link
Contributor

mjmahone left a comment

Only minor questions on why we'd choose optional + nullable keys in the type system

@@ -398,15 +403,15 @@ export type ObjectTypeDefinitionNode = {
name: NameNode,
interfaces?: ?Array<NamedTypeNode>,
directives?: ?Array<DirectiveNode>,
fields: Array<FieldDefinitionNode>,
fields?: ?Array<FieldDefinitionNode>,

This comment has been minimized.

@mjmahone

mjmahone Dec 6, 2017

Contributor

We aren't going to support the empty-set definition, are we? Because it's still defined in the SDL as

ObjectTypeDefinition : Description? type Name ImplementsInterfaces? Directives[Const]? FieldDefinitions?

FieldDefinitions : { FieldDefinition+ }

So this should probably just be

fields?: Array<FieldDefinitionNode>,

right? Or is there a reason allowing it to be both unset and nullable is an advantage I'm missing?

This comment has been minimized.

@leebyron

leebyron Dec 6, 2017

Collaborator

Actually I think due to recent Flow changes we may be able to do this instead.

Previously field?: Array<FieldDefinition> meant that if field was defined, it must be an Array, but now I think it means field may be defined with the value undefined which is very useful for cases where you're writing a utility like {field: otherThing.field}. I'll investigate that and follow up with a separate PR

This comment has been minimized.

@leebyron

leebyron Dec 6, 2017

Collaborator

@mjmahone I tightened up the AST definitions as you suggested plus made them covariant in #1121

};

export type FieldDefinitionNode = {
kind: 'FieldDefinition',
loc?: Location,
description?: ?StringValueNode,
name: NameNode,
arguments: Array<InputValueDefinitionNode>,
arguments?: ?Array<InputValueDefinitionNode>,

This comment has been minimized.

@mjmahone

mjmahone Dec 6, 2017

Contributor

I'm not entirely clear on why this is now a nullable and optional argument. Is this the differences between:

some_field

vs

some_field()

vs

some_field(someArg: ArgType, someOtherArg: ArgType)

?

Basically, in this case, what's the difference between an unset arguments vs arguments: []?

This comment has been minimized.

@leebyron

leebyron Dec 6, 2017

Collaborator

This was fixing a mistake to bring this into consistency with other places where an array of things is optional.

some_field() is a syntax error, so we only care about the difference between some_field and some_field(arg: Type).

This mirrors the same thing with field selection sets where field {} is a syntax error, and we care about the difference between field and field { subfield }.

In both of these cases, we allow the AST to provide an unset property as a short-hand convenience to mean the same thing as that property being defined with an empty array [].

Our parser always produces [] for these to ensure the same hidden class, but the flow types ensure any code that operates on the AST is resilient against the use of this convenience which makes it easier to build ASTs from other utilities and provides some flexibility in the future should we want to have our parser omit empty arrays if it ever becomes a memory burden

@leebyron leebyron force-pushed the sdl-improvements branch from c2f42fc to 0e85d0d Dec 6, 2017

SDL Spec changes
This adds the recent changes to the SDL proposal.

@leebyron leebyron force-pushed the sdl-improvements branch from 0e85d0d to 9f7a0a4 Dec 6, 2017

@leebyron leebyron merged commit d0bb4d8 into master Dec 6, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.05%) to 98.794%
Details

@leebyron leebyron deleted the sdl-improvements branch Dec 6, 2017

leebyron added a commit that referenced this pull request Dec 6, 2017

leebyron added a commit that referenced this pull request Dec 7, 2017

danez pushed a commit to danez/graphql-php that referenced this pull request Feb 11, 2018

Daniel Tschinder
SDL Spec changes
This adds the recent changes to the SDL proposal.

ref: graphql/graphql-js#1117

@danez danez referenced this pull request Feb 12, 2018

Merged

Port 0.12.3 changes from graphql-js #248

0 of 45 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment