Closed yaacovCR closed 2 days ago
Name | Link |
---|---|
Latest commit | c4524ff03f1d358ef1560a8a434b44753fbb7106 |
Latest deploy log | https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/667c26ca3cddad0008dc01e7 |
Deploy Preview | https://deploy-preview-4117--compassionate-pike-271cb3.netlify.app |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
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 π
closing for now after discussing with @robrichard offline => we can revisit later if need be
This makes id an immutable property of each pending result, reducing the need to track these ids / lazily create them within the publisher.
Spec changes
This would change the language in the specification around creating a pending result to be something like:
And retrieving the {id} to be as simple as:
As opposed to the current way of doing things, where we kind of hand-wave and just write when retrieving the {id} for the first time (i.e. when preparing the entry within the
pending
list to send to the client:And then when using it later within
incremental
andcompleted
:Implementation Differences
With this change, empty and filtered deferred fragments will lead to "missing" ids and id ordering will now depend on timing/early execution. The id by the specification are opaque strings so that this is not a specification concern. It's not a practical concern either -- in fact, one could argue that this wierdness is actually beneficial, in that if the ids were always sequential, clients might think that the spec says they should be, but the spec actually says they are opaque strings.
A practical concern, however, is that every future change to internals which results in a different execution ordering (presumably rare, but possible) may result in a change to these identifiers, which could end up breaking library users who have hardcoded their tests utilizing graphql-js with specific identifiers. This seems bad.
Along a very minor similar vein, it is unclear what to label this change. It's just polish, really, but it unfortunately does have an observable difference. Presumably we would have to bump it up to patch-release worthy, which would translate to the bug-fix label.[^1]
My Conclusion
I am leaning against this change.
[^1]: Of course, the actual release type doesn't matter, as we are on alpha and have breaking changes already within our alpha, as one might expect. It's the ambiguity of what to label this change that is parallel to the practical concern above, the theoretical question of how to release if this was the only change in the queue, not actually how we would perform the next release.