Repository navigation
Add syntax for empty objects #1245
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
Open
martinbonnin
wants to merge
7
commits into
empty-object-types
Choose a base branch
from
empty-syntax
base: empty-object-types
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
220e8d4
Allow empty selection sets
benjie 21a2a8c
Allow empty object types
benjie 6dc3236
Subscription type must define at least one field
benjie 9194c66
Merge branch 'main' into empty-object-types
martinbonnin a3d66f4
Add syntax for empty objects
martinbonnin 9e7d6f1
Distinguish between empty types and type declarations
martinbonnin 9700ec4
Revert "Distinguish between empty types and type declarations"
martinbonnin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benjie pointed out in #1249 (comment) that this declaration is incomplete so I think we should remove the second example here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DaleSeo I think both syntaxes should be valid. As a matter of facts, it's already possible to do example 2 today: https://spec.graphql.org/draft/#ObjectTypeDefinition
type Emptyis valid.See also #598 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martinbonnin True, it parses today, but it fails validation because it must define one or more fields. So, the question is whether it should start passing as an empty type.
Interestingly, Lee calls
interface Foo"an incompletely defined interface" in the comment you linked, which lines up with @benjie's take. I'm fine either way, but I'd like all four PRs to reach the same conclusion. Maybe we should settle this with the WG?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference is that it should. I'd be very surprised if
type Empty {}was valid buttype Emptywasn't.👍 Before empty types,
interface Foowas an incompletely defined interface. With empty types, it becomes an empty interface, equivalent tointerface Foo {}.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah Lee explicitly doesn't want this, and though I didn't agree at first, having thought about it, I do agree.
I explicitly do not want two ways of defining an empty object. Thus, I used our existing syntax:
^ User with no fields. This keeps
type User {}as invalid, so there's only one way of defining an empty object type.However,
type User(no braces) has always been an extension point - an indication of things to come later. It's a placeholder, and people who, until now, use it in SDL are expecting to complete the type later viaextend type.We should keep this meaning, and there should remain exactly one way to define an empty object type. Without this, users might accidentally start putting empty types into their schema, and there's no syntactic way to tell a linter "I explicitly mean for this to be an empty type" vs "this is a placeholder I shall complete later". I wish we'd not used
type Userto model this, and instead useddeclare type User, but that ship has long sailed.So:
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I've updated that pull request to reflect that. Let me know how that sounds and I'll update the interfaces PR symmetrically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I've asked internally if we have any evidence people use this pattern. Would be nice to have data to back this.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve been discussing this with Dale in Discord and effectively union is where this argument falls down. There’s no way to define a union that contains no types separately to a union “placeholder” that’s waiting for extension. We could accept that union is the odd duck here and push ahead with empty braces, or we could consistently say that empty types are effectively unadorned, as I do in the parent PR as it stands. The latter has the advantage of not being a syntax change, and thus not breaking any existing parsers. I’m leaning towards the latter, but we can discuss at the WG on Thursday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I put this PR back to its initial state where
type Emptyis a shorthand fortype Empty {}in case we want it. Some robots might end up producing{}by removing all the field definitions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martinbonnin, to close the loop, at the WG meeting yesterday, we agreed on keeping
type Emptyas an incomplete placeholder and usingtype Empty {}for an empty type. @benjie updated #1228 to match, and I did the same for #1249 and graphql/graphql-js#4934.Your point about tools returning
{}after stripping fields fits this approach well, since{}is now valid output. The syntax this PR adds is covered by #1228, so feel free to close this PR.