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

Use cache.batch within cache.updateQuery and cache.updateFragment #8696

Merged
merged 6 commits into from
Sep 17, 2021

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Aug 24, 2021

Since cache.updateQuery and cache.updateFragment (#8382) run user-provided code (the update function), there could end up being multiple cache write operations within a single update, which would trigger broadcastWatches separately, unless we apply batching.

Fortunately, the ApolloCache class already has a tool for broadcastWatches batching: cache.batch (introduced by #7819 in AC3.4). However, the exercise of using cache.batch for this purpose led me a useful improvement for that API, which I describe below.

The cache.batch API followed its predecessor cache.performTransaction(cache => ...) in not returning the result of the transaction function (the options.update function for cache.batch), which makes the following code harder to write:

// This is what I want to write
return this.batch({
  update(cache) {
    // ...
    return data;
  },
});

// This is awkward
let result;
this.batch({
  update(cache) {
    // ...
    result = data;
  },
});
return result;

Since the options.update function is required in Cache.BatchOptions, and cache.batch always calls it once (synchronously) before returning, I think it makes sense for cache.batch({ update(cache) { ... }}) to return whatever options.update returns, enabling the shorter style of code above.

With these improvements to cache.batch, the changes within cache.updateQuery and cache.updateFragment are shorter/cleaner than they otherwise would have been.

Comment on lines -69 to +73
optimistic: string | boolean;
optimistic?: string | boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

Side fix: since optimistic defaults to true, it should not be a required property.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add | undefined here? There’s a lot of TypeScript stuff which distinguishes missing and undefined these days (microsoft/TypeScript#43947)

@@ -472,6 +473,8 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
// options.onWatchUpdated.
this.broadcastWatches(options);
}

return updateResult!;
Copy link
Member Author

@benjamn benjamn Aug 24, 2021

Choose a reason for hiding this comment

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

The ! assertion is warranted as long as perform is called earlier in the function (which it is).

@benjamn benjamn requested a review from brainkim August 24, 2021 22:50
@benjamn benjamn changed the base branch from main to release-3.5 August 24, 2021 22:52
@benjamn benjamn force-pushed the make-cache.batch-return-options.update-result branch from 77756c9 to fea2404 Compare August 24, 2021 22:53
benjamn added a commit that referenced this pull request Aug 24, 2021
benjamn added a commit that referenced this pull request Aug 26, 2021
@benjamn benjamn force-pushed the make-cache.batch-return-options.update-result branch from 9c3cb9b to 184d6af Compare August 26, 2021 22:34
benjamn added a commit that referenced this pull request Aug 26, 2021
@benjamn benjamn force-pushed the make-cache.batch-return-options.update-result branch from adc9b88 to 1b944e6 Compare August 26, 2021 22:41
@benjamn benjamn changed the title Make cache.batch return the result of calling options.update Use cache.batch within cache.updateQuery and cache.updateFragment Aug 26, 2021
benjamn added a commit that referenced this pull request Aug 27, 2021
@benjamn benjamn force-pushed the make-cache.batch-return-options.update-result branch from 1b944e6 to 5c957cb Compare August 27, 2021 17:39
@benjamn
Copy link
Member Author

benjamn commented Aug 27, 2021

@brainkim I added some concrete usage of the new cache.batch behavior (building on #8382), along with a bunch more tests, and rewrote the PR description, if you want to take a(nother) look.

@hwillson hwillson added 2021-09 and removed 2021-08 labels Sep 7, 2021
@benjamn benjamn force-pushed the make-cache.batch-return-options.update-result branch from 5c957cb to cfb2d88 Compare September 17, 2021 17:52
Copy link
Contributor

@brainkim brainkim left a comment

Choose a reason for hiding this comment

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

Some nits but I’m okay to merge. I’m always worried about TypeScript stuff because we aren’t testing that we aren’t breaking TypeScript inference for codegen users (yet), but it doesn’t look like there are any breaking type changes.

@benjamn benjamn merged commit 8ff2f49 into release-3.5 Sep 17, 2021
@benjamn benjamn deleted the make-cache.batch-return-options.update-result branch September 17, 2021 21:00
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
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.

3 participants