Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): Fix OperationContext._identity not allowing operation contexts to be deeply cloned #2732

Merged
merged 4 commits into from
Oct 14, 2022

Conversation

kitten
Copy link
Member

@kitten kitten commented Oct 10, 2022

Resolve #2726

Summary

While we don't imply or implicitly disallow cloning OperationContext, and it's definitely not expected and encouraged, if it's done, it can have some unexpected consequences.

Specifically, if an OperationContext is deeply cloned rather than spread, then OperationContext._identity is destroyed. This leads to the Client not being able to find the right mutation result for a given operation with an unknown _identity.

Previously, we assigned identities with an empty array ([]). There are alternatives, of course, like {}, symbols, functions, etc. But all may be instances that will be swapped out by users.
Instead, we now assign a numeric ID to the field, which is marked as an opaque type in TypeScript. This is a rolling-over 32-bit counter, which is sufficient to prevent mutation results from being confused.

More importantly, this change makes OperationContext serializable in all cases again.

Set of changes

  • Replace OperationContext._identity with a number ID based on a 32-bit roll-over

@kitten kitten requested a review from JoviDeCroock October 10, 2022 23:19
@changeset-bot
Copy link

changeset-bot bot commented Oct 10, 2022

🦋 Changeset detected

Latest commit: 11f9f15

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@urql/core Patch
@urql/exchange-graphcache Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@kitten kitten merged commit 3dd23be into main Oct 14, 2022
@kitten kitten deleted the fix/allow-cloning-context branch October 14, 2022 09:13
@urql-ci urql-ci mentioned this pull request Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some Operations never complete if auth exchange merges headers wrong
2 participants