apollographql / specs

Apollo Library of Technical Specifications
https://specs.apollo.dev/
10 stars 6 forks source link

(nullability) Allow @catch directive to apply all usages of interface field in an operation #47

Open rohandhruva opened 8 months ago

rohandhruva commented 8 months ago

Can we consider extending the nullability spec to allow applying the @catch to an interface type, and have it apply to all fields in that operation which implement that interface? Maybe using schema extensions if that's a better fit.

Here's an example of what our schema looks like:

interface PageRow {
  entitiesConnection: RowEntitiesConnection
}

interface RowEntitiesConnection {
  edges: RowEntitiesEdge 
}

interface RowEntitiesEdge {
  index: Int
  node: RowEntity # this is also an interface
}

type VideoRow implements PageRow {
  entitiesConnection: VideoRowEntitiesConnection
  edges: VideoRowEntitiesEdge
}

type VideoRowEntitiesEdge implements RowEntitiesEdge {
  index: Int
  node: VideoEntity
}

## Imagine similar types for, say, `GamesRow`, `GamesRowEntitiesEdge` and `GameEntity`.

And then in our operations, we use spreads to

fragment RowData on PageRow {
  ...GenericRow
  ...VideoRow
  ...GameRow
}

fragment GenericRow on PageRow {
  ## fetch `index` via the connection -> edge
}

fragment VideoRow on PageRow {
  ## fetch video entities
}

fragment GameRow on PageRow {
  ## fetch game entities 
}

Currently, if I want to @catch something at an entity level, I would need to end up adding the @catch at every single usage of the PageRow, which can be over 20-25 rows for a given operation.

As a result, I think we could add a @catch for, say, RowEntitiesEdge, and allow it to "apply" to all the concrete field usages, I think it would allow us to use that directive in a more straightforward and clean way.

martinbonnin commented 8 months ago

This should be addressed with https://github.com/apollographql/specs/pull/48. To always catch to RESULT on RowEntitiesEdge.node, this should do:

extend interface RowEntitiesEdge @catchField(name: "node")

It'll be available in the 4.0.0-beta.5-SNAPSHOT versions once CI terminates

rohandhruva commented 6 months ago

@martinbonnin , thank you! I'm getting around to trying this now :) I had a quick question, does this apply to union types as well?

We tend to use unions as our return types to give us flexibility to change it in the future, which means that we need to use inline fragment spreads even when we know that there's only one possible object type this union could have. This leads to us having to deal with nullable inline fragment spreads.

martinbonnin commented 5 months ago

Hi 👋

Apologies for the late response. @catch only applies to fields and unions have no fields so it wouldn't make so much sense:

union Node = Product | Review

extend union RowEntitiesEdge @catchField(name: /*there is no field to put here*/)

if an object or interface has a node type. You can mark it as @catch

type Query {
  node: Node
}

# This works
extend type Query @catchField(name: "node")

us having to deal with nullable inline fragment spreads.

I think this is typically what you want? Same as UNKNOWN__ for enums, your app need to handle new members added to the union so it's more robust to handle the default case.

rohandhruva commented 5 months ago

Thank you for explaining, @martinbonnin ! I think that makes sense.