-
Notifications
You must be signed in to change notification settings - Fork 1.1k
RFC: __fulfilled meta field #879
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
base: main
Are you sure you want to change the base?
Conversation
Some early feedback here @mjmahone, could be very well I am missing something. Please correct me where I got something wrong. My concern regarding this PR is that it seems to elevate fragments into something that execution engines need to preserve. This would be a big change to GraphQL with huge (potential breaking) impact. The logic behind this "resolving all fragments" is based on the merged field validation: it basically merges all fields together and resolves all type conditions into Objects so that you only have a tree of fields. Here are a bunch of tests which make this hopefully clearer. My gut feeling is that any improvements should come from In general I am also not sure relying on the specific selections sets/fragments is great. Looking at your example above regarding the fragment A on Viewer {
actor {
__typename
name
...B @include(if: $use_b)
... on Business @include(if: $use_business) {
org
}
}
}
fragment B on User {
org
} You ask where org comes from: is it a Business Org or a User org. I am wondering: if this information is important for the client, why not explicitly ask for it in the query, for example by aliasing it? fragment A on Viewer {
actor {
__typename
name
...B @include(if: $use_b)
... on Business @include(if: $use_business) {
businessOrg: org
}
}
}
fragment B on User {
userOrg: org
} |
@andimarek I don't think this is correct; even after deep merging of the fragments either the field exists in the selection set or it doesn't - it doesn't actually matter that it was inside a particular fragment since it's just a field that always returns |
I understood it as the label Argument allows you to uniquely identify the selection set, which would mean you need preserve the selection set. But maybe this is not correct. |
No, it's just an argument to work around another way that Relay behaves 😉 Specifically Relay's cache kind of ignores aliases so DISCLAIMER: this is based on my recollections of the WG meetings and not an actual understanding of how Relay works, so it may not be accurate or at least might not represent the current public version of Relay 🤷♂️ |
Thanks for the explanation @benjie |
I am a huge, huge supporter of getting this proposal merged as well!
FWIW, Apollo's normalized cache works the same way. Aliases are stripped before storing in the cache. My only comment here is that the label argument seems to be a little bit clunky. I understand the reason for its usage, as @benjie explains.
But it seems redundant to require both the alias and the label argument. I don't love that you need to put both the alias and the argument ( The simple proposal I'd make is that the The downside here is that this would have to be handled specifically as a new edge case by all implementations of course. While the current proposal really just works with the way GraphQL works today. But if it's being added to the spec as a standard meta-data field, it might not be too bad to make that trade off. The implementation of this case would be a pretty trivial addition for most tools. It's also new domain specific knowledge of some magic that just sort of happens under the hood and is a bit opaque, so I understand the arguments against this. But I'd argue the Additionally, if that argument is optional and it's not supplied, normalized caches are going to treat all Perhaps there is a better solution for identifying the scope of the |
Thanks for your input @AnthonyMDev, it's good to get an insight into Apollo's usage as it is one of the widest used clients! By the way, in case you've not been keeping up with the GraphQL Working Group discussions, there's a lot of discussions ongoing at the moment about the place of fragments within GraphQL that cascade into lots of topics. I suspect the WG will be looking for a holistic solution that addresses as many concerns as possible in the most elegant way we can; this RFC may or may not be part of that solution, we need to explore the problem space more before we can figure it out I think - if this is of interest then have a look through the recent WG notes/videos and get involved in the relevant discussions 👍 |
Proposal: Add a
__fulfilled(label: String): Boolean!
meta-fieldThis is currently a Stage 0: Strawman proposal, but I'm including a PR with edits and a
graphql-js
implementation such that this can get knocked down or advance to Stage 1 quickly. @mjmahone is currently championing this proposal.Implemented in graphql-js PR 3216.
Details: Add the below after 4.1: Type Introspection
GraphQL supports the ability to introspect into whether any given selection was included in a response. When a selection is included, all of that selection's fields will be explicitly set, except those not executed due to a directive such as
@include
or@skip
. Selection introspection is accomplished via the meta-field__fulfilled(label: String): Boolean!
on any Object, Interface, or Union. It always returns the valuetrue
:__fulfilled
will be set if and only if the selection containing it is included in the response.For example, given the operation:
The response should be either:
or
This meta-field is often used to clarify response interaction: as an example, the
__fulfilled
field can be used as a way to tell the consumer whether a specific fragment was included in the response. The optionallabel
argument may be used to differentiate between__fulfilled
selections.As a meta-field,
__fulfilled
is implicit and does not appear in the fields list in any defined type.Note:
__fulfilled
may not be included as a root field in a subscription operation.End Spec Addition: Discussion continues below
Why?
I've discussed in the past about how Facebook is moving from product developers interacting with Response-shaped models that use inheritance such that any field in a selection set and all of its children are available. Instead, we try to use composition such that there is one model a product can interact with per selection set, and models need to explicitly convert (via a special conversion method) from parent to child selection sets.
For example, in combination with the
@defer
specification, when we have GraphQL that looks like:An example of well-typed fragment models for the user to interact with, in pseudo-typescript, might look like this:
In this case,
asUserProfilePic()
should returnnull
ifUserProfilePic
has not yet been fulfilled in a response payload. Unfortunately, to determine that, we'd need to reach outside our local fragment, and check the response'slabel
. Even that seems a bit buggy: what ifUserWithName
is included in multiple locations in the operation's tree?The cleanest solution I've thought through is having a well-defined boolean field inserted, at the location we care about, that will be true if and only if the selection set we care about is included in the response. Note, this solution applies not just to
@defer
, but also to existing directives like@include
and@skip
, and could even enable simpler handling of type resolution. All a client needs to do to check whether a selection set was included, is to add something likeFoo__fulfilled: __fulfilled(label: "Foo")
, and see if the response containsFoo_fulfilled: true
.Client Specific Considerations
On most of our clients today (including Relay), we parse data from the server into a Graph format: we merge every instance of a single unique record (usually as determined by a primary key's value like
id
), taking all fields requested across an entire operation (or many operations), and merge them into a locally consistent graph.However, product developers interact with this local graph as though it is shaped like the GraphQL selection sets described by their operation and fragment definitions. Meaning even though the server response might look like:
there may be a variety of fragments, with drastically different shapes, that describe how specific components use that underlying response:
Just by looking at the structure of the response, I don't know whether my component using A should have access to
actor.org
: if bothUser
andBusiness
are interfaces, how can we tell whether this is the User's org or the Business' org? Or both?Today, in order to determine whether
org
is fromfragment B
or... on Business
, we have clients that do one of:$use_b
and$use_business
sent with the original requestOpenSourceContributor
is aUser
orBusiness
__typename
-aliasing hack, and re-route aliased typenames to a client-only field for our local Graph.What to call this boolean meta-field?
I am open to bikeshedding here. Some alternatives:
__exists
: the field is for figuring out whether a selection set exists, so this feels natural__id
: it's almost a "selection set identity" function. This is probably not a good name, as it will clash with lots of other implementations where__id
means "identifier" rather than the "identity functionid(x: X): X
".isFulfilled
: this makes it clear we're using the field to answer a question. The downside is it's a camelCase'd meta-field, where many implementations use a snake_case style guide. I would greatly prefer a name that works well under either camelCase or snake_case paradigmsI am open to other suggestions!
Alternative 1: Just use
__typename
Using
__typename
is an option. However, because__typename
is of typeString!
, it's a complicating option: the response is bloated with unnecessary information, and any infra that relies on this field needs to translate the response as a string to a boolean value, based on whether the key exists rather than the actual__typename
value.Concretely: it's much less error-prone to generate models that, under the hood, do boolean checking on boolean-returned values, rather than requiring either a field-existence check, or a string-exists-and-is-not-empty check (depending on client language).
Furthermore, if we are using a normalized, canonical store for our response data, as Relay and Apollo and other clients do, we need a special fulfilled field handler just to make sure we don't accidentally write
is_Foo_fulfilled
to the canonical__typename
, and ask whether__typename
exists. Note, this is usually how we attempt to solve the problem today, and it has lead to actual bugs, hence this PR.With the
__fulfilled(label:)
argument, the canonical version of the field can be used to differentiate unique selection sets, allowing theasUserProfilePic
implementation from the Why? section to become:Alternative 2: Don't use a meta-field, if you want this ability explicitly add the field to each type in your schema
We could do this, but then we can't use this field on Unions. Also, if a field exists on every type, it should probably be a meta-field.
Other Alternatives
This is a straw man, so I'd be very interested in how other people solve this problem, and whether the above proposal would make their implementations better or be completely useless or harmful.