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: SDL - Separate multiple inherited interfaces with & #1169

Merged
merged 1 commit into from
Dec 20, 2017

Conversation

leebyron
Copy link
Contributor

@leebyron leebyron commented Dec 20, 2017

This replaces:

type Foo implements Bar, Baz { field: Type }

With:

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 inheritance. To allow for an adaptive migration, this adds a parse option to continue to support the existing experimental SDL: parse(source, {allowLegacySDLImplementsInterfaces: true})

@leebyron
Copy link
Contributor Author

leebyron commented Dec 20, 2017

Optimistically applied this change to the pending spec proposal (graphql/graphql-spec#90) in graphql/graphql-spec@32b55ed

@leebyron leebyron force-pushed the sdl-implements-multiple-interfaces branch from 4041853 to a930eec Compare December 20, 2017 01:44
@leebyron
Copy link
Contributor Author

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

@dschafer
Copy link
Contributor

Conceptually feels super reasonable to have a symbol used here. The & symbol choice didn't resonate with me at first, but it matches very closely with the concept at https://flow.org/en/docs/types/intersections/#toc-intersections-of-object-types so it seems like a reasonable choice to me.

I have some reservations about how many symbols we're introducing for the SDL (& and """ come to mind), but they all seem to be used in a relatively constrained manner (so we're not losing that much option value), so it doesn't bother me that much.

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, implements is the symbol. Not sure I like that better, but it is another option.

@dschafer
Copy link
Contributor

To allow for an adaptive migration, this adds a parse option to continue to support the existing experimental SDL: parse(source, {legacySDL: true})

I'd prefer to see this gated under sdlWhitespaceForMultipleInterfaceInheritence or the like, since I doubt this is the last time we'll make a change to the SDL.

@freiksenet
Copy link
Contributor

freiksenet commented Dec 20, 2017

This looks great! I agree with @dschafer on specific SDL parsing flags instead of generic ones.

@dschafer
Copy link
Contributor

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.

@leebyron
Copy link
Contributor Author

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)

@stubailo
Copy link

This looks great, glad it eliminates the ambiguity!

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})`
@leebyron leebyron force-pushed the sdl-implements-multiple-interfaces branch from c772768 to f317a65 Compare December 20, 2017 20:05
@leebyron leebyron merged commit c496fb7 into master Dec 20, 2017
@leebyron leebyron deleted the sdl-implements-multiple-interfaces branch December 20, 2017 20:42
tonyghita added a commit to graph-gophers/graphql-go that referenced this pull request Mar 7, 2018
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
tinnywang pushed a commit to tinnywang/graphql-go that referenced this pull request Dec 13, 2018
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
cpmsmith added a commit to fellowapp/graphql-core-legacy that referenced this pull request Apr 18, 2022
Equivalent to graphql/graphql-js#1169
(excluding the 'allowLegacySDLImplementsInterface' option)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants