artsy
[WIP] Increase non-nullability of connections
eigen recently enabled TypeScript's strict mode. After doing that we noticed a lot of places in the MP schema where things that should be non-nullable are instead nullable. Happily going from nullable->non-nullable is a backwards compatible change, so we've set aside some time to do that in places where it would be most impactful. This is the first PR contributing to that effort.
This PR affects four things
- Makes connection types non-nullable where possible (the only nullable connection types I found were connections like
followedArtiststhat only make sense for logged-in users) - Makes the edges array non-nullable (it's always there when we use
connectionFromArraySlice, which we use everywhere in MP) - Makes the edges themselves non-nullable (again, they're always present with
connectionFromArraySlice) - Makes the edge nodes non-nullable where possible (I couldn't find any examples where nullability made sense, but I added a mandatory property for the config object to force people to choose either way when creating new connection types.)
This doesn't affect connections which were stitched in from other services, we'd have to update those manually.
This looks awesome! Increasing type safety, and making things non nullable that can be is a great backwards compatible change. The one caveat is that instead of a node 'silently' being null (and a client possibly already handling it properly), there will now be that 'Cannot return null for non-nullable field...' entry in the GraphQL errors response, and the entire parent node will be null (which may cause client instability where previously there may have been none).
Makes connection types non-nullable where possible (the only nullable connection types I found were connections like followedArtists that only make sense for logged-in users)
Is this actually correct? I think a lot of those logged-in fields generally have the pattern of if (!loggedIn) return null near the top of their resolver (some even raise a custom "Must be logged-in..." error instead of returning null). Would adding non-nullability to those effectively add a third type of error that would arise if the user is logged out (the GraphQL non-null validation one)?
Makes the edges array non-nullable (it's always there when we use connectionFromArraySlice, which we use everywhere in MP) Makes the edges themselves non-nullable (again, they're always present with connectionFromArraySlice)
Makes sense to me!
Makes the edge nodes non-nullable where possible (I couldn't find any examples where nullability made sense, but I added a mandatory property for the config object to force people to choose either way when creating new connection types.)
The nodes are usually derived from RESTful GET list endpoints, which shouldn't be returning null entries in the list, so I would agree that feels like a reasonable restriction. Of course we've all been surprised by the data before!
The one caveat is that instead of a node 'silently' being null (and a client possibly already handling it properly), there will now be that 'Cannot return null for non-nullable field...' entry in the GraphQL errors response, and the entire parent node will be null (which may cause client instability where previously there may have been none).
Good thinking. For the changes in this PR, i believe that could only potentially happen at the connection->edges->[edge]->node position, and only if gravity returns null items in a result array. I checked every single endpoint touched here and they all seemed to avoid nils by the way they select and filter data, e.g. the snippet Artworks.where({artist: foo... is not gonna be returning any nils.
Some endpoints had fairly opaque logic to me though (as a non-rubyist), so I couldn't be 100% sure in all cases. Do you think it would be prudent to make doubly sure this won't bite us by explicitly filtering out nils in gravity?
Is this actually correct? I think a lot of those logged-in fields generally have the pattern of if (!loggedIn) return null near the top of their resolver (some even raise a custom "Must be logged-in..." error instead of returning null). Would adding non-nullability to those effectively add a third type of error that would arise if the user is logged out (the GraphQL non-null validation one)?
Exactly, that's why I left those particular connection fields alone. My wording was not very clear about that in the bullet point you were replying to, apologies.
For every gravity-backed connection field in MP, I read through the resolver code to check whether they could potentially return null and I only made the ones I was 100% sure could never return null non-nullable. If memory serves, the only fields that could potentially return null were the ones that required users to be logged in, the ones whose resolvers use the if (!loggedIn) return null pattern you described, which I left alone.
I'm going to manually test this PR against eigen and reaction before submitting for review again.
Thanks for clarifying, loving this PR!
e.g. the snippet Artworks.where({artist: foo... is not gonna be returning any nils.
Some endpoints had fairly opaque logic to me though (as a non-rubyist), so I couldn't be 100% sure in all cases. Do you think it would be prudent to make doubly sure this won't bite us by explicitly filtering out nils in gravity?
Nah, I sort of tend to agree that shouldn't ever happen.
If memory serves, the only fields that could potentially return null were the ones that required users to be logged in, the ones whose resolvers use the if (!loggedIn) return null pattern you described, which I left alone.
Makes sense and thanks for the clarification! I skimmed the diff and was noticing that some connections which were user-scoped had the nonNullable option added, and those did seem to follow that return null pattern. Might have slipped thru! Generally anything under me, or referencing follows or notifications would be user-scoped (and thus subject to that nullability, potentially), and I did notice some nonNullable configs added in such-named files, which is what prompted the question.
@mzikherman I've tested this out locally with force and eigen and it all seems good, everything works as before.
The one problem is that reaction is on an older version of relay and/or the relay typescript plugin which apparently means that this change would break some of the typings in reaction (__typename fields on edge node types become a generic string instead of remaining as the actual type names)
I checked and upgrading all the relay things in reaction does indeed fix this issue but I don't have time to finish that job before I'm on vacation for over a week 😞 I'll leave this open
I loved reading all of this PR and can’t wait to see the eigen PR making use of these changes 🤩
I forgot to add an update here, thanks for the bump @alloy 😅
This PR is still blocked by force. I tried to upgrade relay in force after reaction was retired but it didn't seem to fix the problem with union __typenames this time. I spent a little while debugging relay-compiler-language-typescript but didn't have time to get to the root cause before sprint work caught up with me. I'll add a ticket to our backlog to get this tracked so it doesn't just sit here forever.
The issue does ring a bell for me, did you try with the latest version of the plugin? Or perhaps it’s related to the version of relay itself? Alternatively, can you point out what code fails?
yeah I updated to the latest versions of everything. Not sure where the failure is but the symptom is that in
someConnection {
edges {
node {
__typename
}
}
}
..., where the node type is a union of concrete types, the emitted TS type for the __typename field is a string union of all the concrete type's names when edges and node are nullable. But when they're non-nullable the emitted type is plain old string.
I tried debugging the relay-compiler bin running against force and had trouble wading through the noise, so I then tried to create a regression test in the relay-compiler-language-typescript repo. Seems I got sidetracked before getting anywhere with that.
I bet there’s a codepath in the plugin that isn’t unboxing a union of non-nulls in the same way it would a union of nullable.
This never got done 😢 but y'all should give it another go at some point! Gonna close it to clear my open PRs.