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

[WIP] Non-nullable error boundary - interrobang #1046

Closed
wants to merge 1 commit into from

Conversation

benjie
Copy link
Member

@benjie benjie commented Sep 21, 2023

Inspired by @captbaritone's True Nullability Schema discussion (graphql/graphql-wg#1394) and following @fotoetienne's excellent talk at GraphQLConf, and trying to "use the things we already have in new ways" I am proposing that we introduce a new non-nullable variant, the "Non-Null error boundary" type, represented via interrobang mark !?. This will be a Non-Null type in the current sense, but with an extra property of __Type.isErrorBoundary: true

Critically, this type would "evaporate" for legacy clients, appearing the same as a nullable field. (This is enabled via the includeErrorBoundaries argument to the __Field.type field, which defaults to false.)

The main difference in this type is that it will never be null unless there is an error in the errors array. So it's like a hybrid between the nullability default, and Non-Null. A middle ground.

I have not made all the spec edits for this, I'm just feeling out the problem space right now. "Collecting" the field error (rather than throwing it) is going to need some careful wording... but that's a problem for when we come to formalize it.

@netlify
Copy link

netlify bot commented Sep 21, 2023

Deploy Preview for graphql-spec-draft failed.

Name Link
🔨 Latest commit 3311df3
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/650cd1b4e6e0f500080d720a

@captbaritone
Copy link

First of all, thanks for kicking off the discussion. I think this proposal gets us what we will need for True Nullabilty, and severs as a great starting place for iteration.

Things I like about this proposal:

  • Provides a mechanism for both simple clients and error handling clients to “see” the nullabiltiy they want to present to their users
  • We can easily experiment with this approach using a directive without confusing existing/legacy schema tools (maybe we could consider an official directive that is equivalent to the syntax version in order to enable backwards compatibility?)
  • Existing syntax maintains existing semantics. Interrobangs can be added gradually to enable better type generation error handling clients without any behavioral changes affecting existing clients

Things I wish could be better:

Mainly the syntax has some drawbacks ( perhaps we should defer syntax bike shedding for another time, but I’ll list a few concerns for now)

  • Assuming that we keep our existing “nullable by default” best practice for non-error-handling clients, this two character syntax (barring Unicode cuteness) is going to be very common and introduce considerable visual noise to our schemas
  • (As you mentioned in our in-person discussion) we are headed toward a world with three types of nullabilty. Purely from a syntax perspective, ideally we could avoid multi-character annotations and could use:
    • ! Never null (existing non nullable behavior)
    • ? True nullabilty
    • null only on error (the behavior you are proposing here)
  • However, backwards compatibility precludes this 🙁

An observation: we’ll also need to consider how this new type variant would affect interface validation and potentially selection validation.

@benjie
Copy link
Member Author

benjie commented Sep 23, 2023

I agree on all your points @captbaritone. From a separate in-person conversation, I think @mjmahone thinks that this new "non-nullable error boundary" type should be the default, but I'm not convinced I agree - I think nullable+null boundary (i.e. the current default) should remain the default.

Also, I'm considering changing it into a different "wrapper type" instead. So these spec edits are far from complete.

@martinbonnin
Copy link
Contributor

Agree that the syntax is a bit weird. Is !? used in any other language?
Also the list case is always a bit awkward:

type User {
  friends: [User!?]!?
}

But overall, what bugs my brain the most here is that it introduces some kind of dissymmetry between nullable and non-nullable types. Why would error boundaries be limited to non-nullable types?

I get the technical reason why (null bubbling is only an issue for non-nullable fields). But from a user point of view, I'd expect errors and nullability to be orthogonal concepts. If we're saying !? is an indication that a non-nullable field might error, I would almost expect a symmetrical ??that hints that the given nullable field might error. But maybe it's too much?

@benjie
Copy link
Member Author

benjie commented Sep 23, 2023

Yeah, so what I’m considering is that you’d have e.g. errorBoundary(nonNullable(Int)) which would come out as Int!?. The problem right now is that error boundaries are implicit, so this would have a pretty tough migration story for schemas.

@benjie benjie changed the title [WIP] True nullability schema - interrobang [WIP] Non-nullable error boundary - interrobang Sep 26, 2023
@BoD
Copy link
Contributor

BoD commented Sep 28, 2023

To clarify, with this schema:

type Query {
	book: Book!?
}

type Book {
	author: User!?
}

querying book when the author resolver fails, would return:

a/

{ "errors": {...}, "data": { "book": { "author": null } } }

or b/

{ "errors": {...}, "data": { "book": null } }

?

@benjie
Copy link
Member Author

benjie commented Sep 28, 2023

(A) (there is no bubbling past an interrobang )

@benjie
Copy link
Member Author

benjie commented Oct 4, 2023

Closing this in favour of the much more complete asterisk proposal: #1048

@benjie benjie closed this Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants