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

feat(graphcache): track types in the data-structure #3501

Merged
merged 18 commits into from
Mar 2, 2024
Merged

Conversation

JoviDeCroock
Copy link
Collaborator

@JoviDeCroock JoviDeCroock commented Feb 3, 2024

Related to #3470

Summary

This starts tracking a mapping of __typename to a list of entityKey, this enables us to perform operations like invalidate('Todo') which would invalidate every entityKey of the Todo type.

We add non-root typenames when going through the write operation and we clean up entities in the list when we are going through gc runs.

In the future this can instruct us for operations where we can't derive an entity i.e. a create-mutation will invalidate all entities of its own type to ensure we don't run into stale values which leaves us with the scalar return case which won't automatically be handled.

The PR explicitly does not add the updater logic yet so we can see that the explicit API surface does not grow, we support more cases instead.

Set of changes

  • Add Map of __typename to a list of keys
  • Add support for invalidate('Type') and inavlidate({ __typename: 'Type' })

Questions

Should we support invalidating a full type with field and arguments i.e. invalidate('Todo', 'author') would go through every Todo key and explicitly invalidate the Author field.

Follow-ups

  • After this PR we can provide logic where we check the presence of the entity in cahce when there's no updater. When we can't find the entity we perform invalidate({__typename})

Copy link

changeset-bot bot commented Feb 3, 2024

🦋 Changeset detected

Latest commit: 8125b32

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

This PR includes changesets to release 1 package
Name Type
@urql/exchange-graphcache Minor

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

@@ -170,18 +170,27 @@ export class Store<

invalidate(entity: Entity, field?: string, args?: FieldArgs) {
const entityKey = this.keyOfEntity(entity);
const shouldInvalidateType =
Copy link
Collaborator Author

@JoviDeCroock JoviDeCroock Feb 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should narrow this down to only supporting invalidate({ __typename: 'Todo' }) as this explicitly can't derive a key nor be a global-id which can be the case with invalidate('x').

EDIT: getting quite sure it's safer to just do { __typename: 'X' } as with global-ids we would other wise have to check whether the resulting key is present in the cache and assume it's a typename when not

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be problematic if people accidentally pass invalidate an entity that happens to be embedded.

This would issue warnings in other places, but if ignored, sometimes, given incomplete fragments for instance, a user could accidentally pass an object that contains no key fields just in that moment.

However, we could allow invalidate('TypeName'), since we can check for types.has(typeName) pretty easily now.

I see below though that we check for Object.keys(x).length. I'm not sure if there's ever a case where someone wants to pass cache.invalidate({ __typename: 'X' }) over just a string?

Might be API-bloat 🤔 Unsure

@JoviDeCroock JoviDeCroock marked this pull request as ready for review February 4, 2024 16:02
exchanges/graphcache/src/store/data.ts Outdated Show resolved Hide resolved
exchanges/graphcache/src/store/data.ts Outdated Show resolved Hide resolved
Copy link
Member

@kitten kitten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing that stops us from merging ❤️ All just nit comments and follow-up questions

.changeset/tall-buttons-fetch.md Outdated Show resolved Hide resolved
exchanges/graphcache/src/operations/invalidate.ts Outdated Show resolved Hide resolved
@@ -238,6 +240,7 @@ export const make = (queryRootKey: string): InMemoryData => ({
hydrating: false,
defer: false,
gc: new Set(),
types: new Map(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this could potentially be faster in some cases as a normal object. That's because, while we avoid objects for entity key properties, using objects for anything predictable (field names and type names) should potentially be faster

exchanges/graphcache/src/store/data.ts Outdated Show resolved Hide resolved
exchanges/graphcache/src/store/data.ts Outdated Show resolved Hide resolved
@@ -170,18 +170,27 @@ export class Store<

invalidate(entity: Entity, field?: string, args?: FieldArgs) {
const entityKey = this.keyOfEntity(entity);
const shouldInvalidateType =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be problematic if people accidentally pass invalidate an entity that happens to be embedded.

This would issue warnings in other places, but if ignored, sometimes, given incomplete fragments for instance, a user could accidentally pass an object that contains no key fields just in that moment.

However, we could allow invalidate('TypeName'), since we can check for types.has(typeName) pretty easily now.

I see below though that we check for Object.keys(x).length. I'm not sure if there's ever a case where someone wants to pass cache.invalidate({ __typename: 'X' }) over just a string?

Might be API-bloat 🤔 Unsure

@JoviDeCroock JoviDeCroock merged commit 492af08 into main Mar 2, 2024
7 checks passed
@JoviDeCroock JoviDeCroock deleted the track-types branch March 2, 2024 13:42
@github-actions github-actions bot mentioned this pull request Mar 2, 2024
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.

None yet

2 participants