Replies: 8 comments 33 replies
-
I'm feeling like Aliased Spreads are my favorite at the moment. It feels like a logical extension of the mechanics already available in GraphQL. Since the basis for this discussion is Relay, would you be down to provide an example Relay component? Especially one at the level of complexity where we start to see the problems that this RFC is trying to resolve. I think it will help with discussion. |
Beta Was this translation helpful? Give feedback.
-
Example 1: Testing If an Inline Fragment MatchedConsider a component that displays an entity, which might be an function Profile(props) {
const entity = useFragment(
graphql`
fragment Profile_user on Node { // Node is an interface
... on Actor { // Actor is an interface
name // String!
}
id
}
`,
props.entity,
);
// goal: render the name if the entity was an Actor, else null.
if ( /* entity is Actor */ ) {
const name: string = /* get entity as Actor */.name;
return name;
}
return null;
} As you can see, it isn't clear how to test that the Status QuoIn the above case Relay currently emits a Flow type (or similar TypeScript) type as follows:
Note that querying function Profile(props) {
const entity = /* ... same ... */
// goal: render the name if the entity was an Actor, else null.
if (entity.hasOwnProperty('name')) { // in practice you might also need a null check on the value too
const name: string = entity.name;
return name;
}
return null;
} Ideal: Check a named propertyIdeally though, we'd have aliased inline fragments and named fragments, so you could just name the function Profile(props) {
const entity = useFragment(
graphql`
fragment Profile_user on Node { // Node is an interface
asActor: ... on Actor { // Actor is an interface. The alias could also be automatic based on the type
name // String!
}
id
}
`,
props.entity,
);
// goal: render the name if the entity was an Actor, else null.
if (entity.asActor != null) {
const name: string = entity.asActor.name;
return name;
}
return null;
} Where here the result type would be described by the following Flow/TS type:
Here, it's very clear that if the Example 2: For Fragment SpreadsThis also works well for fragment spreads. Consider that we want to render a child component only if its fragment was evaluated (only if its type matched — though this could work well for spreads that are condition w eg function Profile(props) {
const entity = useFragment(
graphql`
fragment Profile_user on Node { // Node is an interface
asActor: ...ActorProfile_actor // given "fragment ActorProfile on Actor", where Actor is an interface
id
}
`,
props.entity,
);
// goal: render <ActorProfile> if that fragment matched (ie entity was an Actor
if (entity.asActor != null) {
return <ActorProfile actor={entity.asActor} />;
}
return null;
} Again, the presence of Status QuoThere isn't a great workaround for the fragment spread case today. Developers would basically have to do: function Profile(props) {
const entity = useFragment(
graphql`
fragment Profile_user on Node { // Node is an interface
...ActorProfile_actor // given "fragment ActorProfile on Actor", where Actor is an interface
... on Actor { // This type has to be kept in sync w the fragment definition **manually**
isActor: __typename
}
id
}
`,
props.entity,
);
// goal: render <ActorProfile> if that fragment matched (ie entity was an Actor
if (entity.hasOwnProperty('isActor')) {
return <ActorProfile actor={entity} />;
}
return null;
} Note how we have to manually add an extra inline fragment, the type of which has to be kept in sync w the fragment's type manually. Gross, and brittle. |
Beta Was this translation helpful? Give feedback.
-
I don't have any strong opinions about which outcome is better. I was a bit surprised, though, to not see the benefits of field merging mentioned as part of the tradeoffs involved. While field merging certainly adds a lot of complexity to GraphQL and gets in the way of some of the functionality described in the RFC, it also means that you can put together a page's query by cobbling together potentially-overlapping fragments from subcomponents and not have to worry about whether you'll be getting identical data repeated many times on the wire. So I think understanding which potential solutions may end up bloating response sizes with repeated data would be helpful. |
Beta Was this translation helpful? Give feedback.
-
In terms of fragment modularity and different response formats, I think it's important to note explicitly that the introduction of the defer/stream directives creates a different response format with an error boundary (graphql/defer-stream-wg#23), or, at least it does as currently implemented. The only reason that it does not offer a more complete solution to fragment modularity is that the new response format is strongly recommended, but ultimately optional, if a server feels it can otherwise provide better performance., so fields within a deferred fragment can still not conflict with other fields. (There is also the separate problem of fulfillment, of course....) Mentioning this just for completeness of the problem space, really, not (cough, cough) heaven forbid, because I am still pushing that the directives be mandatory... More seriously, any new format we have above should take into account how defer/stream will be handled. For instance, in @josephsavona 's examples above: fragment Profile_user on Node { // Node is an interface
isActor: ... on Actor @defer { // Actor is an interface
name // String!
}
id
} isActor might not end up in the initial response because the node is not an Actor, or because the server agreed to defer isActor as recommended. Presumably, in the case of a deferred fragment, we would need to substitute some placeholder special "pending" type value to indicate that more is to come. Perhaps "true" would work, as we could already indicate to the client that we know the node is an actor, although this is only natural with the appropriately named alias as above. Anyway, the overall proposal looks incredibly interesting, would love to follow along/participate. |
Beta Was this translation helpful? Give feedback.
-
Thank you for putting together this proposal! I recognize the problem statement provided is valid, and I believe we should be working on solutions. The Apollo iOS Client and our users struggle with these issues constantly. The approach of modularizing fragments under their own response keys seems to be the clear favorite, but I am concerned about the duplication of data with this approach (FWIW, I think making it opt-in via aliases is definitely the way to go if we use this approach). But I worry that every time someone hits a validation conflict, they will just opt-in to modularity and make their operations less performant. Start nesting those modular fragments at large scale and you are going to duplicate a lot of data for the sake of developer convenience. One of the big selling points of GraphQL is reducing data OTA by fetching only what you need. This solution, if implemented, is going to actively encourage duplication of data in way too many places.
While this is true, there are always trade offs to performance. This proposal removes developer-side validation checks speeding up build times, but incurs a greater run-time cost. Execution of duplicated data is going to increase CPU cycles and data transport. I think that’s generally a bad trade-off to make. Fragment spreads as they work today were intended to provide for the modularity we are seeking here. But as we’ve seen, there are serious limitations concerning whole-program validation and alias conflicts. But providing opt-in for breaking fragments out into duplicated data sets seems heavy-handed to me. I’d like us to explore more possibilities of how to resolve the conflicts prior to data duplication. Perhaps there is a way to create unique keys for conflicting fields in response data and have clients map each of those fields to the correct response key at run time? I am not outright against this proposal, but I do think we should do a deeper dive into other possible solutions before moving in this direction. Starting with an understanding all of the cases in which conflicts can cause validation errors, we can determine if there are ways for servers and clients to be smarter about resolving them. I am, however, strongly in favor of the |
Beta Was this translation helpful? Give feedback.
-
@AnthonyMDev: great point, let's do a deep dive on validation issues! I'm going to create another top-level response to kick off that deep dive. We can keep discussions about response size I think inline with the other top-level responses. OverviewThere's a few reasons validation rules are non-modular today. I'll classify all our validation rules into 5 buckets, ranked from, in my opinion, most-modular to least-modular. There are, by my count, 26 Executable Definition validation rules. Of these, 17 are already fully modular (Bucket 1). 4 rules could be made compatible with standard package/module based build systems, if we created the concept of "definition declarations" (Bucket 2) and "imports" for fragment spreads (Bucket 3). 3 more rules are not modular because fragments do not define each variable they use as part of their "definition declaration" (Bucket 4). Bucket 4's rules could be made modular if we had the declarations and imports from Buckets 2 and 3, and also required fragments to declare all the variables they use. The final set is a single rule, that Fields in a Set must be able to merge. Specifically, that fields in a set must be able to merge with fields in a selection set's fragment spreads, recursively (Bucket 5). This one rule is what makes fragments hopelessly non-modular, even if all of the above is solved. All of the other buckets could be solved without changing the response shape: we would need to potentially change GraphQL's syntax, but that changed syntax should only need to modify validation, rather than the query's execution algorithm. But for Field Set Merging Across Fragments, we need a way to know fields cannot end up overlapping with incompatible fields in order to solve. 1: Rules requiring only a definition and the declarations of its first order fragment spreadsFor these, we can run the validation entirely knowing only: the contents of a definition, and the definition declaration of any fragment it spreads. What I mean by a definition declaration, is the contents of a definition that come before the opening selection set. That includes:
So this would include the bolded portion of the definitions:
The 17 rules that fit this category are:
2: Rules requiring knowing the declaration of all definitionsThese rules require knowing each definition's declaration, but do not require knowledge about the definition's selection set contents. The 3 rules are: 3: Rules requiring both the declaration and fragment spreads of each definitionThese rules are those that require we know the dependencies of all definitions, i.e. which fragments they spread, and may need to know the declaration of every definition too. The 2 rules are: 4: Rules that need to know what variables a fragment usedBecause fragments do not define all the variables they use, during validation we need to keep track of all the variables that were defined, and also what variables a specific fragment uses, in order for these rules to work. However, if we always included these variable usages in a fragment's definition declaration, we could make these modular provided we solved buckets 2-3 too. The 3 rules are: 5: Rules that require knowing the contents of the selection sets of all definitionsThese are the least modular: there's no gimmicks you can do to avoid looking through every field in every definition and keeping track of some mutable state in order to come to the right answer for these validation rules. There's only a single rule in this set. The 1 rule is: |
Beta Was this translation helpful? Give feedback.
-
We're happy to discuss offline, but I also think it's important for us to be clear about this to the broader GraphQL community, so i'll try to answer here (also, it's performance review time at Meta and this is the perfect way to procrastinate btw writing reviews). Our goal for our GraphQL tooling is to be as fast and responsive as possible in order to keep developers in the flow of building their app. This impacts how we think about designing our build tools and our IDEs. In general our GraphQL clients promote writing GraphQL as small fragments colocated with UI/product code that describe the specific data dependencies of that product code, which then compose (via fragment spreads) the dependencies of any child UI components or product code that it may call. Let's refer to these as colocated fragments, though of course at the root of a UI surface you might have actually have a query that defines the fields needed by the root of your UI and spreads for child components. For our build tools, our goal is to allow developers to quickly change their colocated fragments, ie to change their data selections, and then preview the updated application as quickly as possible. This translates to re-producing as few artifacts as possible to preview the updated version of the application: eg any generated classes, type definitions, or other artifacts necessary to rebuild. So for example we generally try to avoid rebuilding the entire GraphQL corpus, and instead rebuild only the artifacts that could actually be affected by the change to the fragment. That's our current state of the art: to rebuild all queries and fragments which are transitively referenced by any operation which (transitively) references the changed fragment. For highly shared fragments, this can be a ton of queries and fragments. Our goal is that we would, in the common case, rebuild exactly one artifact: the class/type definition/ast/other representing the fragment itself, without even having to look at the definition of any other fragment or operation. And by common case I mean changing the selections of the fragment without changing its signature: if fragments were truly modular, then changes to selections alone could never invalidate build artifacts from other fragments. For our IDE tooling, our goal is to near-instantaneously return all validation errors across the codebase as a result of the user's change: the earlier that our tooling provides feedback, the easier and faster it is for a developer to address. Here, it's clear that if fragments are not modular, then any change to a fragment's selections could cause invalidation errors anywhere that fragment is transitively referenced, requiring checking all those places. So just like with builds, we similarly want to make fragments modular so that by design, changes to selections only does not require running validation rules on anything other than the fragment definition in isolation (you have to check that any fragment spreads are valid, but you only have to look at their signature, not their contents). I hope this helps to clarify why the solutions you've proposed don't achieve our goal: they require an additional build stage in which we must look and transform the contents of the transitive fragments referenced by operations, and not just the fragment that was changed. One thing that is probably not obvious from the above is: how do you actually run the updated queries using the new fragment content? If our build tools only process the changed fragment, that would seem to imply that we haven't constructed updated operations referencing it, and therefore can't run the app and fetch the correct data. The catch here is that we use persisted queries: our build tools tell the server about each document that exists on the client. When we build a changed fragment, we similarly inform the server about the updated version of that fragment. In development mode, the server could then dynamically construct a query from the latest version of each fragment. This of course only works if we know that the query would be valid, which again relies on making fragments truly modular. But when you put this all together, you get: near-instantaneous builds, near-instantaneous IDE feedback, and rapid iteration at dev time. For production builds we can of course take extra time to pre-build each operation on the server to avoid overhead. So the solution has to make fragments truly modular — we need to avoid needing any form of transitive rewriting phase on operations affected by fragment changes. That hopefully clarifies why we are exploring the specific set of solutions:
|
Beta Was this translation helpful? Give feedback.
-
This is all very interesting. I wanted to toss in a link to this issue/conversation where I discuss a closely related problem set and raise some very rough potential solutions. In it, @leebyron provided some context around the history and thought behind the current situation, which I found very helpful and might be useful here as well. |
Beta Was this translation helpful? Give feedback.
-
See This RFC for more details: https://github.com/graphql/graphql-wg/blob/main/rfcs/FragmentModularity.md
In an ideal world, fragments would be so modular that our validation rules would be inductive: if Query Q depends on Fragment B and C, then so long as we know fragments B and C are valid, and we run validation on Q against B and C's type declarations (i.e. just the
fragment B on Foo
portion of the fragment definition, the stuff before the selection set brackets{ ... }
), then we know Q is valid.I'm hoping this discussion helps us clarify the work required to set us in that direction, and makes sure related projects, like Client Controlled Nullability, are at least compatible with this evolution.
Currently I see three different "solution spaces" for addressing this problem:
Personally, I'm of the opinion that the metadata approach is a cudgel: it might give clients some hooks to help solve client-specific problems, but doesn't give us a clear path towards solving other, related problems like Error Boundaries. So I'm more focused on the other two paths: my favored approach would be one where you could safely opt-in to everything being modular.
To get the discussion started, cc @josephsavona @captbaritone @hwillson @twof who have all explicitly expressed interest in this area.
Beta Was this translation helpful? Give feedback.
All reactions