graphql-js

further refine incremental result types

#4323

Open

Not sure what we are trying to achieve here but should we be explicit about what these types can be?

i.e. if Data is always going to be an object should we use extends Record<string, unknown>

Furthermore TItems sounds like it's always going to be an array so why don't we make this a TIncrementalItem and wrap it in the ReadonlyArray ourselves? Is the purpose of this wrapping so that folks can avoid passing in a readonly one? If so then we still need to push people into the happy path by constraining the inputs.

Hmmm, basically, I ran into type conflicts when working on the transformer in #4319 because TData was mapped to TItems, and this was a simple fix for that.

Taking a step back, I think we should honestly consider removing all of the generics from the incremental results. As far as I understand, the generic parameters more generally are used by tools like GraphQL CodeGen to provide a typed experience for the ExecutionResult based on the individual query, but it is possible/likely that typed access to the incremental data will be from the merged object anyway, after all the incremental bits have been merged in, and we don't need to provide these generics at all.

I think we may have simply started off supplying them because the IncrementalDeferResult is modelled after an ExecutionResult, and the ExecutionResult provides this. @JoviDeCroock @robrichard @benjie any thoughts?

I think it's an anti-pattern to leave untyped bits, I can see a world where the incremental result is typed as

type PossibleIncrementalResults = 
  | UserFragment
  | TodoFragment

Which would then be supplied to this generic as the deferred/streamed bits are the only possible combinations of return values for a specific IncrementalExecutionResult. It makes sense to split off the initial and incremental results into two generics as we are sure about the shape of the initial payload but uncertain about the shape of the incremental results as they will be many and variadic based on the places they occur.

Just a factoid to add to this, because of inlining we are not 100% on the shape of the initial result either.

If the shape of the response, be that initial isn't predictable. Isn't that a big deviation from the typed nature of graphql?

I understand that it might be eventually a consistent shape but I'd reckon that the initial shape would be consistent. I understand that defer might not be respected and that it might just be the full thing from the start. We can convey a bail in types though where if the first result indicates subsequent result it will be the minimum viable shape, when the reverse is indicated it is the final shape. When the incremental results indicate being in a done state it will also be the final shape, all that combined makes for a deterministic picture afaict.

The important thing to me is being accurate and living up to the typed nature of GraphQL.

The shape of the response is as predictable as a request using @skip and @include where the value of the variables isn't factored into codegen.

Gotcha, yes, I thought I was missing something so

because of inlining we are not 100% on the shape of the initial result either.

is just the addition of i.e. optional properties

c72159a