Skip to content

Leaf field selections clarification #958

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

Merged
merged 8 commits into from
Jun 17, 2022

Conversation

benjie
Copy link
Member

@benjie benjie commented Jun 5, 2022

An alternative proposal to one of the changes in #957, this clarifies that we're talking about the "unwrapped type" or "underlying type" of the fields/selections.

@benjie benjie added the ✏️ Editorial PR is non-normative or does not influence implementation label Jun 5, 2022
@netlify
Copy link

netlify bot commented Jun 5, 2022

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit f2699c5
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/62acee98a3c1a8000817b6d2
😎 Deploy Preview https://deploy-preview-958--graphql-spec-draft.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@leebyron
Copy link
Collaborator

I think "result type" instead of 'underlying type" is a more correct term to use. I reframed the explanatory text to more directly mirror the algorithm.

@benjie let me know what you think

@benjie
Copy link
Member Author

benjie commented Jun 17, 2022

Thanks for looking this over @leebyron! The term "result type" is only used once in the spec right now AFAICT and it's here. Imagine we have a selection set { foo } which contains a selection foo of type [[Int!]!]!:

type Query {
  foo: [[Int!]!]!
}

query Q { foo }

If you asked me what the "result type" of the selection foo was, I'd assume it was the type the foo resolver would need to return, that being [[Int!]!]!. Though the unwrapped result type is a scalar, I'm uncomfortable saying that [[Int!]!]! is a scalar (not least because mathematically, [[1, 2], [3, 4]] is not a scalar).

We have this kind of "the type includes wrapped versions of the type" wording throughout the spec text:

the leaves of this tree are typically GraphQL Scalar types (but may also be Enum types or null values).
-- https://spec.graphql.org/draft/#sel-EAHXFCA_DCCJkwQ

A GraphQL Input Object defines a set of input fields; the input fields are either scalars, enums, or other input objects.
-- https://spec.graphql.org/draft/#sel-DAHhBDJAACLAy3B

We also have places where we are more explicit, for example:

A field of an Object type may be a Scalar, Enum, another Object type, an Interface, or a Union. Additionally, it may be any wrapping type whose underlying base type is one of those five.
-- https://spec.graphql.org/draft/#sel-FAHZpBABAB6Ft4T

In the algorithm text we tend to be much more precise, though thinking about it, maybe that's simply by virtue of always checking if it is a non-null or list type first, so by the time it hits "is an Object type" it's already unwrapped... 🤔

I used the term "underlying type" influenced by:

A wrapping type has an underlying named type, found by continually unwrapping the type until a named type is found.
https://spec.graphql.org/draft/#sel-GAHVTJABqDB9GqE

Maybe we can solve this by defining "result type" to be this "underlying named type" (e.g. Int)? If we do, we must have a name for the actual type the selection uses though (e.g. [[Int!]!]!), and I fear calling that the "type" may lead to ambiguity, and "return type" would be confusing 😉

@rivantsov
Copy link
Contributor

rivantsov commented Jun 17, 2022

agree with @benjie that 'return type' would be confusing for this particular use, unwrapped or underlying type seem better

@leebyron
Copy link
Collaborator

Cool, I agree that "unwrapped" is more accurate and more consistent with how we refer to this kind of type elsewhere.

My thought with "result" type was clarifying the type of value returned, as opposed to the type's involved with inputs or any other element of a field. Given the context that's probably unnecessary.

Thanks both of you for championing this clarification, it's much better

@leebyron leebyron merged commit 1b8fe7a into main Jun 17, 2022
@leebyron leebyron deleted the leaf-field-selections-clarification branch June 17, 2022 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✏️ Editorial PR is non-normative or does not influence implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants