Simplify collectFields for @defer & @stream #3982
Conversation
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Hi @robrichard, 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:
|
robrichard
force-pushed
the
defer-stream-collectFields
branch
2 times, most recently
from
1648183
to
b520452
Compare
|
Hey @robrichard! Could we take a look at this rebased on #3984 after that is reviewed/merged => ideally there would be no/minimal test changes at that point... |
robrichard
force-pushed
the
defer-stream-collectFields
branch
3 times, most recently
from
0da8e48
to
355ca68
Compare
current loop assumes that the first analyzed DeferredFragmentRecord has been released as pending and thereofre has an `id`, which happens to be the case in this scenario, but is not reliable, uncovered by #3982. Demonstrates the peril of `!` which might point to requiring additional types, but we can leave that for a different PR. Another option is to include an `invariant` call -- that should be unnecessary, but would indeed be safer.
robrichard
force-pushed
the
defer-stream-collectFields
branch
2 times, most recently
from
a8994d9
to
9c5ba6e
Compare
robrichard
force-pushed
the
defer-stream-collectFields
branch
from
9c5ba6e
to
a2adbc1
Compare
we only use the parent because masking analysis done differently
| for (const fieldDetail of fieldDetails) { | ||
| if (fieldDetail.deferUsage != null) { | ||
| deferUsageSet.add(fieldDetail.deferUsage); | ||
| newDeferUsages.add(fieldDetail.deferUsage); |
There was a problem hiding this comment.
In the current implementation, collectFields() returns:
groupedFieldSet: GroupedFieldSet;
newGroupedFieldSetDetails: Map<DeferUsageSet, GroupedFieldSetDetails>;
newDeferUsages: ReadonlyArray<DeferUsage>;I just realized that in addition to removing newGroupedFieldSetDetails, this PR also removes newDeferUsages and so your list of potential newDeferUsages is going to include every field nested under every defer. Because you have changed addDeferredFragments to check the deferMap before actually creating a new Deferred Fragment, you don't end up with duplicate records, but there is a lot of duplicate effort.
Maybe it might make sense to allow collectFields() to return newDeferUsages again and just get rid of newGroupedFieldSetDetails?
| } else { | ||
| let deferredGroup = deferredGroups.find((group) => | ||
| isSameSet(group.deferUsageSet, deferUsageSet), | ||
| ); |
There was a problem hiding this comment.
because masking here is done differently, this find is not necessarily stable, consider the operation:
query HeroNameQuery {
... @defer(label: "DeferID") {
hero {
id
}
}
... @defer(label: "DeferName") {
hero {
name
lastName
... @defer {
lastName
}
}
}
}hero is used in two parallel defers, with the id subfield present in DeferID and both name and lastName present in DeferName. The subfields of hero should therefore yield two new grouped field sets, but according to this algorithm, it would yield three, as the additional superfluous defer would still be counted and so the deferUsageSet for lastName would actually be different for name.
There was a problem hiding this comment.
to fix this, you may need to bring back ancestors on DeferUsage
There was a problem hiding this comment.
I raised an alternative approach in #3994.
minimizes changes to CollectFields a la graphql#3982 but still creates a single memoized incremental field plan per list item.
minimizes changes to CollectFields a la graphql#3982 but still creates a single memoized incremental field plan per list item.
minimizes the changes to `CollectFields` required for incremental delivery (inspired by graphql#3982) -- but retains a single memoized incremental field plan per list item.
No description provided.