urql-graphql / urql

The highly customizable and versatile GraphQL client with which you add on features like normalized caching as you grow.
https://urql.dev/goto/docs
MIT License
8.54k stars 444 forks source link

docs(graphcache): remove mutative operations #3503

Closed JoviDeCroock closed 4 months ago

JoviDeCroock commented 5 months ago

It's confusing that we use mutative operations in our updaters when that can alter the base-layer when we i.e. are working with optimistic layers.

changeset-bot[bot] commented 5 months ago

⚠️ No Changeset found

Latest commit: 8d5a9bec5446deb5b92f5465b73549ccb2784492

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

kitten commented 4 months ago

I see the issue here, so basically, a user could accidentally modify a link on a layer by mutating it directly, since cache.resolve returning links is an exception, i.e.: https://github.com/urql-graphql/urql/blob/f06ef3499b69a20896057c0027c5116f4e73c54a/exchanges/graphcache/src/store/store.ts#L161-L162

Does it make sense for us to instead clone links in cache.resolve? It should be the only place that can cause this problem and we already have an appropriate function that can clone links deeply: https://github.com/urql-graphql/urql/blob/f06ef3499b69a20896057c0027c5116f4e73c54a/exchanges/graphcache/src/operations/shared.ts#L226-L249

JoviDeCroock commented 4 months ago

I would personally prefer hammering on the immutable route in the docs either way

kitten commented 4 months ago

I'm fine with changing the docs, yea, let's get this merged as a docs change then, and I'll file the fix above separately, so we don't let this bug become a longer term problem 👍