further refine incremental result types #4323
Conversation
continue work with graphql#4313
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
| TData = ObjMap<unknown>, | ||
| TItems = ReadonlyArray<unknown>, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| TodoFragmentWhich 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.
There was a problem hiding this comment.
Just a factoid to add to this, because of inlining we are not 100% on the shape of the initial result either.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
continue work with #4313