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

- Documentation: added some details about cache updates #11152

Merged
merged 5 commits into from
Jan 22, 2024
Merged

- Documentation: added some details about cache updates #11152

merged 5 commits into from
Jan 22, 2024

Conversation

JonasDoe
Copy link
Contributor

@JonasDoe JonasDoe commented Aug 17, 2023

Documentation: added clarification to function Modifier API.
I'm not a native speaker, though, so some errors might have slipped through. Additionally, my adjustments are made on what I understood from the discussioin in the linked issue. If I misunderstood something there, it will be faulty here. :)

fixes #11135

Checklist:

  • If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

- Documentation: added clarification to function Modifier API
@JonasDoe JonasDoe requested a review from a team as a code owner August 17, 2023 10:22
@apollo-cla
Copy link

@JonasDoe: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@netlify
Copy link

netlify bot commented Aug 17, 2023

‼️ Deploy request for apollo-client-docs rejected.

Name Link
🔨 Latest commit 5e1fbd8

@changeset-bot
Copy link

changeset-bot bot commented Aug 17, 2023

⚠️ No Changeset found

Latest commit: 5e1fbd8

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

@JonasDoe
Copy link
Contributor Author

JonasDoe commented Aug 17, 2023

I can't check why Build and Test is failing. Some formatting requirement which passed the linter, though?

@Meschreiber
Copy link
Contributor

Thanks for this @JonasDoe! I reran the Build and Test check and it looks alright now. I'll review the docs changes next week, apologies for the delay!

Copy link
Contributor

@Meschreiber Meschreiber left a comment

Choose a reason for hiding this comment

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

Thanks so much for these additions @JonasDoe ! I'd love to clarify the bits in docs/source/caching/cache-interaction.mdx and am calling on @bignimbus for assistance 🙏

docs/source/api/cache/InMemoryCache.mdx Outdated Show resolved Hide resolved
You can use whichever combination of strategies and methods are most helpful for your use case.
You can use whichever combination of strategies and methods are most helpful for your use case.

Changing cache fields with these methods will only affect the objects at the very place you specify by the query, the fragment or the location of the `modify` call. So, if you're changing an object at a nested place from `{__ref: '5'}` to `{__ref: '5', completed: true}`, this will override the `completed` value for cache reads at the specific place, but nowhere else.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused by the use of "place" here: do you mean the particular reference?

@bignimbus , since you were part of the original discussion maybe you have some insight?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With "place" I mean what I'm specifying with a fragment or query, e.g. an object with id: 5 in a result at 'ROOT_QUERY.myEntities("orderBy": "size", "pagination": {"first": 20, "cursor": 1234})'. Changing a value of this object there won't affect the object with the same __ref/id in, say, 'ROOT_QUERY.myEntities("orderBy": "age", "pagination": {"first": 20, "cursor": 1234})', and neither the object itself, stored at the root level (__APOLLO_CLIENT__.data.data). At least that's what I understood from the original discussion,

I'm not sure what you mean with "particluar reference" - the Reference object or the pointer to the updated ... eh ... location in the cache. It's really hard to find the right words when everything seems to have a double meaning: a naive one and one specififc for the Apollo Cache domain. :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the tag! Should we consider a different approach here? We have documentation about __ref objects. We could link to this url. Ex:

Suggested change
Changing cache fields with these methods will only affect the objects at the very place you specify by the query, the fragment or the location of the `modify` call. So, if you're changing an object at a nested place from `{__ref: '5'}` to `{__ref: '5', completed: true}`, this will override the `completed` value for cache reads at the specific place, but nowhere else.
Bear in mind the difference between fields that contain references to other objects in the cache and fields that contain literal values. References are objects that contain a `__ref` field (read more [here](https://www.apollographql.com/docs/react/caching/overview/#example)). Modifying a reference will not change the values contained in the object to which the reference points. So avoid updating an object from something like `{__ref: '5'}` to `{__ref: '5', completed: true}`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two things:

  • You're using a double space between sentences. It that common? I think I haven't seen that elsewhere in the documentation.
  • Why should I avoid doing to update an object to {__ref: '5', completed: true}? I my case, I wanted to update a query result with a generic utility method (for modyfing pagination results), so I did exactly that. This generic method has no knowledge about the underlying data or let alone its type, and since cache.modify does not come with a straight-forward way to fetch the referenced item, I went with:
const myModifier: Modifier<Reference> = (prevData, details) => {
   return {
      ... prevData,
      someField: 'updatedValue'
   }
}

and thought: 'Well, for my current component it's enough since my updatedValue will be regarded there, and for other the data will be reloaded anyway!' (depends on the fetch policy ofc.). This is why I created the issue at the first place - b/c I wasn't sure that intended, and by the and, I figured it was. :D

Copy link
Contributor

@bignimbus bignimbus Aug 23, 2023

Choose a reason for hiding this comment

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

Thanks for the reply @JonasDoe! My two cents: there's nothing explicitly wrong with doing what you're doing, it's just not a pattern that I think the maintainers should explicitly support. Reference objects are intended to point to other objects, the fact that they can be arbitrarily extended to include value literals is just a feature of JavaScript, not an intentional design choice of Apollo Client. I hope that clarifies things! Happy to go to single spaces after periods, it's just a habit of mine to do double spaces. I believe HTML will render the same thing either way.

Copy link
Contributor Author

@JonasDoe JonasDoe Aug 24, 2023

Choose a reason for hiding this comment

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

I think I screwed something up. I wanted to reply here with an suggestion, but commited your proposal of this part, as it seems. Anyway, then I try it the old way:
I'ld suggest something as

So avoid updating an object from something like `{__ref: '5'}` to `{__ref: '5', completed: true}` if you want to update more than that specific query result.

to clarify the discouragement.

docs/source/caching/cache-interaction.mdx Show resolved Hide resolved
@Meschreiber Meschreiber requested a review from bignimbus August 21, 2023 19:06
@bignimbus
Copy link
Contributor

Thanks @Meschreiber and @JonasDoe, I'll take a look and respond as soon as I can 🙏🏻

@bignimbus bignimbus self-assigned this Aug 23, 2023
JonasDoe and others added 2 commits August 24, 2023 10:52
Bear in mind the difference between fields that contain references to other objects in the cache and fields that contain literal values. References are objects that contain a `__ref` field (read more [here](https://www.apollographql.com/docs/react/caching/overview/#example)). Modifying a reference will not change the values contained in the object to which the reference points. So avoid updating an object from something like `{__ref: '5'}` to `{__ref: '5', completed: true}` if you want to update more than that specific query result.

Co-authored-by: Jeff Auriemma <[email protected]>
Co-authored-by: Maria Elisabeth Schreiber <[email protected]>
@jerelmiller
Copy link
Member

@Meschreiber and/or @bignimbus just wanted to check in and see if you're satisfied with the most recent changes. If so, I'd be happy to get this merged!

Copy link
Contributor

@Meschreiber Meschreiber left a comment

Choose a reason for hiding this comment

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

Thanks for the ping @jerelmiller ! Just a small nit, otherwise lgtm!

docs/source/caching/cache-interaction.mdx Outdated Show resolved Hide resolved
JonasDoe and others added 2 commits January 20, 2024 16:15
en-dash requires spacing, though: `field – see`

Co-authored-by: Maria Elisabeth Schreiber <[email protected]>
@jerelmiller jerelmiller merged commit 491ec41 into apollographql:main Jan 22, 2024
20 of 24 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vague documentation regarding Modifier and readField
6 participants