refactor(incremental): set pending result id during execution

[yaacovCR]

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:

Let {id} be a unique identifier for this new Deferred Fragment.
Let {path} and {label} be the path and label....
Let {newDeferredFragment} be an unordered map containing {id}, {path} and {label}

And retrieving the {id} to be as simple as:

Let {id} be the corresponding entry on {deferredFragment}.

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:

Let {id} be a unique identifier for this new Deferred Fragment.

And then when using it later within incremental and completed:

Let {id} be the unique identifier for this Deferred Fragment.

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.¹

My Conclusion

I am leaning against this change.

Footnotes

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. ↩

[netlify]

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	c4524ff
🔍 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.

[github-actions]

Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

• @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
• @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

[yaacovCR]

closing for now after discussing with @robrichard offline => we can revisit later if need be