-
Notifications
You must be signed in to change notification settings - Fork 13
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
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
CCN’s ? can save normalized caches from null bubbling corruption #20
Comments
It would be wise for Relay to not only add ? to every non-null field, but to nullable fields to in case the schema evolves (since changing a nullable output field to non-nullable is generally seen as non-breaking). Perhaps a shortcut could be to place a ? after the operation name: |
Good point about schema changes! Ultimately what a client like Relay wants, both for the purpose outlined in this post as well as the True Nullability Schema goal outlined in #19, is an option to opt out of null bubbling globally, exactly as you propose. A global flag like this more elegantly describes what Relay, and other sufficiently-smart normalizing clients, wants. In addition a global flag would be approachable for simpler clients that don't transform a user-defined query. Although, for those middle-tier smart-clients that don't transform, maybe they would prefer that the flag be supplied as a header or http query variable. That way the client could select the execution mode without needing the user's participation (adding the query-level annotation)... I'm was a bit hesitant to propose a change where a top-level annotation forks execution behavior for the full query. Having two separate execution modes sounds undesirable from the perspective of the spec and also the mental overhead of the whole ecosystem. That said, I think the value that could be unlocked by a true nullability schema is considerable and would likely be worth the added complexity. I think the big question here is: For smart clients that can handle errors client-side, a single top-level annotation like you propose would obviate the need for the rest of CCN. Should we consider skipping CCN, with its complexity, limitations, and tradeoffs, all together and try to make the jump straight to True Schema Nullability? Some outstanding questions are:
|
{ // Response B (without null bubbling)
"data": {
"me": {
"id": "10",
"age": {
$$isError: true, // <-- note that this key is not possible through aliases, but could happen in custom scalars... Risky.
"message": "Age errored",
// no need for "path"
} // <-- Errored, but did not bubble!
}
}
} Answer: we can't do this, because of custom scalars. If your |
I see that you answered your own question regarding that particular encoding scheme, but to answer your question more broadly, I think you're spot on. With explicit error handling, Relay is going to start thinking of the response as a single tree that just happens to encode errors in a look-aside array for the purposes of unambiguous JSON encoding. Relay's runtime is sufficiently sophisticated to be able to, in essence, perform that transformation on the client. So, for us, the only advantage of a server response that actually was a single tree, would just be some slight performance advantages when interpreting the server response. |
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
TL;DR: I believe smart clients like Relay, which maintain a normalized cache, could leverage CCN’s
?
to prevent cache corruption due to null bubblingThe Problem
Null bubbling (when a error or null value encountered in a non-nullable field bubbles up to a parent nullable field) is destructive. It causes true and correct data that could be included in the response to be omitted. This can cause problems when trying to write a GraphQL response into a normalized cache.
Imagine two cards A and B side by side, each of which contain information about the current user. A shows “name” and B shows “age”. They are powered by two different queries. The response for query A looks like this:
We can write this into our normalized cache and it will look like:
This is fine, and we can nicely render our first card.
Now, however, we get an error in our second query’s response:
The age field, which was non-nullable, has errored, and we won’t be able to render our second card. Oh well, such is life. However, when we go to write this into our cache, things get worse:
Now card A, which also reads from this normalized store, is broken despite the fact that none of the fields it reads are in an error state.
A possible solution
With the adoption of Relay’s proposed error handling feature, product code is shielded from implicit field errors via framework-level explicit error handling. This means Relay is no-longer dependent on the server’s response shape strictly matching the schema. Specifically, it’s fine for a field marked as an error to be missing, even if it’s non-nullable. In other words, Relay can safely opt-out of server-side null bubbling.
And CCN's
?
offers Relay just that option. Relay’s compiler can annotate every non-null field in the query it generates with a?
to opt out of any null bubbling.Note: I’ve documented the CCN behavior I’m expecting here.
A new world
Lets now imagine how response B would look like in this new world:
When we write this into our normalized cache we get:
Card B still can’t render, but note that we’ve prevented our normalized cache from getting corrupted due to null bubbling. Thanks CCN!
Related
This post is spiritually related to #19 in that they both explore the benefits of a mode of GraphQL execution that avoids null bubbling. It may be worth exploring other mechanisms where-by clients could opt out of null bubbling, but I wanted to point out that CCN’s
?
is a powerful enough primitive to allow a smart client like Relay to opt out of null bubbling.Hat tip to @RyanHoldren who wrote an excellent internal post articulating this normalization issue. My note here is very much inspired by that post.
The text was updated successfully, but these errors were encountered: