feat: Allow returning `IGNORE` sentinel from `optimisticResponse` functions to support bailing-out from the optimistic update #11410

sf-twingate · 2023-12-04T23:29:16Z

Summary

This PR updates the behaviour of the optimisticResponse functional property in mutations to support returning null, which will cause the optimistic update to be skipped / bailed out from.

This is useful to support an optimistic response only when certain variables are provided to the mutation. For example, in my use case, I only wish to perform an optimistic update when a specific variable is not included, as I can't reliably compute the result on the client when it is.

Currently, I can workaround this by writing an additional query without an optimisticResponse property for mutations which use this variable, and then conditionally calling one of the two mutations based on the input variables. However, this results in duplicate code and I feel this small API change would make things simpler and might be useful in other use cases.

Notes

I was going to open this as an issue, but opted to write an implementation to support my request. If there's a better way of implementing this, or if the PR is missing something (new to contributing to the codebase!), just let me know and I'm happy to update it.

apollo-cla · 2023-12-04T23:29:18Z

@sf-twingate: 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 · 2023-12-04T23:29:19Z

👷 Deploy request for apollo-client-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	`ef08c05`

changeset-bot · 2023-12-04T23:29:20Z

🦋 Changeset detected

Latest commit: ef08c05

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

This PR includes changesets to release 1 package

Name	Type
@apollo/client	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

alessbell · 2023-12-06T18:50:03Z

Hi @sf-twingate 👋 Thanks for this draft PR and starting this discussion! We agree that there should be a way to bail out of an optimisticResponse, but have a couple of thoughts before we proceed with this PR.

Since null has semantic meaning in GraphQL, I'd suggest undefined if we stick with a primitive type. But there's another option: returning a sentinel object, an approach we use elsewhere, e.g. DELETE or INVALIDATE sentinels used when interacting with the cache (docs).

Since the current behavior is to always set the cache value to optimisticResponse's return value (() => void results in undefined as well), it's unlikely but possible that someone could be relying on the existing behavior when returning either undefined or null. Unlikely, but possible. The sentinel object approach would be fully backward-compatible and something we could ship in a future minor.

Would you want to implement that change in this PR and convert it out of a draft? The changes should be fairly minimal, and I'd be happy to help if you hit any blockers. Let me know - if not I'd be happy to move this over the finish line. Thanks!

alessbell · 2023-12-11T20:46:36Z

@jerelmiller @phryneas I've gone ahead and moved this out of draft against the release-3.9 branch as discussed. The only change I've made is to use an IGNORE sentinel object instead of null.

A note on docs: I updated the optimisticResponse type but after this is merged I'll add an example for skipping an optimistic update.

Thanks for the contribution, @sf-twingate, this is ready for review 🎉

alessbell · 2023-12-11T21:04:42Z

src/core/QueryManager.ts

@@ -626,6 +633,8 @@ export class QueryManager<TStore> {
        invariant.error(error);
      }
    }, mutation.mutationId);
+
+    return true;


Now that I'm looking at this again, I'd vote to move the data === IGNORE check out of markMutationOptimistic to avoid changing the fn signature here to one that returns a boolean. Thoughts?

I'd be ok with this. I'd prefer that as well 🙂

jerelmiller

Changes are ok by me! @alessbell once you're happy with this, feel free to merge.

@sf-twingate thanks so much for the contribution!

jerelmiller · 2023-12-12T05:40:06Z

.changeset/mighty-coats-check.md

+            // conditionally bail out of optimistic updates
+              return IGNORE;


Suggested change

// conditionally bail out of optimistic updates

return IGNORE;

// conditionally bail out of optimistic updates

return IGNORE;

Minor nit to ensure these are aligned 🙂

jerelmiller · 2023-12-12T05:45:08Z

src/core/QueryManager.ts

@@ -626,6 +633,8 @@ export class QueryManager<TStore> {
        invariant.error(error);
      }
    }, mutation.mutationId);
+
+    return true;


I'd be ok with this. I'd prefer that as well 🙂

sf-twingate · 2023-12-13T17:20:17Z

@alessbell sorry for the late response — had a busy work week and didn't yet have a chance to continue with this PR. Thank you for taking care of updating it!

The changes made LGTM — just let me know if you need anything additional from my end to land this 🎉

…functions to bail out of update

sf-twingate changed the title ~~feat: Allow returning null from optimisticResponse functions to bail-out from the optimistic update~~ feat: Allow returning null from optimisticResponse functions, to support bailing-out from the optimistic update Dec 4, 2023

sf-twingate force-pushed the feature/sf/allow-bailing-from-optimistic-update branch 2 times, most recently from 7e9af0d to 65764d3 Compare December 4, 2023 23:38

phryneas added the 🏓 awaiting-team-response requires input from the apollo team label Dec 5, 2023

alessbell force-pushed the feature/sf/allow-bailing-from-optimistic-update branch from 65764d3 to 18b03bb Compare December 11, 2023 20:15

alessbell changed the base branch from main to release-3.9 December 11, 2023 20:15

alessbell added 🧞‍♂️ enhancement and removed 🏓 awaiting-team-response requires input from the apollo team labels Dec 11, 2023

alessbell requested review from jerelmiller and phryneas December 11, 2023 20:39

alessbell added 🧶 minor-release 🥚 backwards-compatible for PRs that do not introduce any breaking changes auto-cleanup 🤖 labels Dec 11, 2023

alessbell marked this pull request as ready for review December 11, 2023 20:44

alessbell requested a review from a team as a code owner December 11, 2023 20:44

alessbell self-assigned this Dec 11, 2023

alessbell reviewed Dec 11, 2023

View reviewed changes

alessbell changed the title ~~feat: Allow returning null from optimisticResponse functions, to support bailing-out from the optimistic update~~ feat: Allow returning IGNORE sentinel from optimisticResponse functions to support bailing-out from the optimistic update Dec 11, 2023

jerelmiller approved these changes Dec 12, 2023

View reviewed changes

sf-twingate and others added 4 commits December 15, 2023 12:23

feat(QueryManager): allow returning null from optimisticResponse …

42e2388

…functions to bail out of update

feat: update to use IGNORE sentinel object, update changeset

245940a

chore: update type in mutation options documentation

25f15e3

chore: fix merge conflicts in api reports

3dd910d

alessbell force-pushed the feature/sf/allow-bailing-from-optimistic-update branch from 9f8f310 to 3dd910d Compare December 15, 2023 17:24

chore: bump size limits

ef08c05

alessbell removed the auto-cleanup 🤖 label Dec 15, 2023

alessbell merged commit 07fcf6a into apollographql:release-3.9 Dec 15, 2023
27 of 28 checks passed

github-actions bot mentioned this pull request Dec 15, 2023

Version Packages (beta) #11417

Merged

alessbell mentioned this pull request Jan 3, 2024

chore: add docs for skipping an optimistic update via optimisticResponse #11461

Merged

3 tasks

github-actions bot locked as resolved and limited conversation to collaborators Jan 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow returning `IGNORE` sentinel from `optimisticResponse` functions to support bailing-out from the optimistic update #11410

feat: Allow returning `IGNORE` sentinel from `optimisticResponse` functions to support bailing-out from the optimistic update #11410

sf-twingate commented Dec 4, 2023 •

edited

apollo-cla commented Dec 4, 2023

netlify bot commented Dec 4, 2023 •

edited

changeset-bot bot commented Dec 4, 2023 •

edited

alessbell commented Dec 6, 2023

alessbell commented Dec 11, 2023

alessbell Dec 11, 2023

jerelmiller Dec 12, 2023

jerelmiller left a comment

jerelmiller Dec 12, 2023

jerelmiller Dec 12, 2023

sf-twingate commented Dec 13, 2023

		// conditionally bail out of optimistic updates
		return IGNORE;

feat: Allow returning IGNORE sentinel from optimisticResponse functions to support bailing-out from the optimistic update #11410

feat: Allow returning IGNORE sentinel from optimisticResponse functions to support bailing-out from the optimistic update #11410

Conversation

sf-twingate commented Dec 4, 2023 • edited

Summary

Notes

apollo-cla commented Dec 4, 2023

netlify bot commented Dec 4, 2023 • edited

👷 Deploy request for apollo-client-docs pending review.

changeset-bot bot commented Dec 4, 2023 • edited

🦋 Changeset detected

alessbell commented Dec 6, 2023

alessbell commented Dec 11, 2023

alessbell Dec 11, 2023

Choose a reason for hiding this comment

jerelmiller Dec 12, 2023

Choose a reason for hiding this comment

jerelmiller left a comment

Choose a reason for hiding this comment

jerelmiller Dec 12, 2023

Choose a reason for hiding this comment

jerelmiller Dec 12, 2023

Choose a reason for hiding this comment

sf-twingate commented Dec 13, 2023

feat: Allow returning `IGNORE` sentinel from `optimisticResponse` functions to support bailing-out from the optimistic update #11410

feat: Allow returning `IGNORE` sentinel from `optimisticResponse` functions to support bailing-out from the optimistic update #11410

sf-twingate commented Dec 4, 2023 •

edited

netlify bot commented Dec 4, 2023 •

edited

changeset-bot bot commented Dec 4, 2023 •

edited