-
Notifications
You must be signed in to change notification settings - Fork 2k
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: SDL - Separate multiple inherited interfaces with &
#1169
Conversation
Optimistically applied this change to the pending spec proposal (graphql/graphql-spec#90) in graphql/graphql-spec@32b55ed |
4041853
to
a930eec
Compare
@freiksenet, @stubailo, @dschafer - would love your opinion on this change. I think it makes sense in the context of the whole, solves the ambiguity Mikhail pointed out, and offers some tools for migration. I'd like a second opinion to make sure I'm not crazy. If there's some agreement, I'd release this as v0.13 to allow some space for the subsequent SDL change. |
Conceptually feels super reasonable to have a symbol used here. The I have some reservations about how many symbols we're introducing for the SDL ( One other option that I just thought of is that instead of: type Foo implements Bar & Baz { field: Type } we could do type Foo implements Bar implements Baz { field: Type } that is to say, |
I'd prefer to see this gated under |
This looks great! I agree with @dschafer on specific SDL parsing flags instead of generic ones. |
Ah, I may have rederived why you went with a general one - if this is a single flag to refer to the "pre-spec" SDL (e.g. this would also include comments as documentation, for example), then I think I'm fine with it being general. |
Specific flags makes sense in the context of the existing specific flag for comment descriptions. I'm hoping to phase them out over major releases to ease the path to adoption I'm hoping this is the last real change to the SDL before codifying it (though of course, we could expand on it in the future in new proposals) |
This looks great, glad it eliminates the ambiguity! |
a930eec
to
c772768
Compare
This replaces: ```graphql type Foo implements Bar, Baz { field: Type } ``` With: ```graphql type Foo implements Bar & Baz { field: Type } ``` With no changes to the common case of implementing a single interface. This is more consistent with other trailing lists of values which either have an explicit separator (union members) or are prefixed with a sigil (directives). This avoids parse ambiguity in the case of an omitted field set, illustrated by #1166 This is a breaking change for existing uses of multiple inheritence. To allow for an adaptive migration, this adds a parse option to continue to support the existing experimental SDL: `parse(source, {allowLegacySDLImplementsInterfaces: true})`
c772768
to
f317a65
Compare
This replaces: ```graphql type Foo implements Bar, Baz { field: Type } ``` With: ```graphql type Foo implements Bar & Baz { field: Type } ``` This is more consistent with other trailing lists of values which either have an explicit separator (union members) or are prefixed with a sigil (directives). This avoids parse ambiguity in the case of an omitted field set, illustrated by [github.com/graphql/graphql-js#1166](graphql/graphql-js#1166). For now, the old method of declaration remains valid. References: - graphql/graphql-js#1169 - graphql/graphql-spec#90 - graphql/graphql-spec@32b55ed
This replaces: ```graphql type Foo implements Bar, Baz { field: Type } ``` With: ```graphql type Foo implements Bar & Baz { field: Type } ``` This is more consistent with other trailing lists of values which either have an explicit separator (union members) or are prefixed with a sigil (directives). This avoids parse ambiguity in the case of an omitted field set, illustrated by [github.com/graphql/graphql-js#1166](graphql/graphql-js#1166). For now, the old method of declaration remains valid. References: - graphql/graphql-js#1169 - graphql/graphql-spec#90 - graphql/graphql-spec@32b55ed
Equivalent to graphql/graphql-js#1169 (excluding the 'allowLegacySDLImplementsInterface' option)
This replaces:
With:
With no changes to the common case of implementing a single interface.
This is more consistent with other trailing lists of values which either have an explicit separator (union members) or are prefixed with a sigil (directives). This avoids parse ambiguity in the case of an omitted field set, illustrated by #1166
This is a breaking change for existing uses of multiple inheritance. To allow for an adaptive migration, this adds a parse option to continue to support the existing experimental SDL:
parse(source, {allowLegacySDLImplementsInterfaces: true})