Urql: Should custom updates triggered by optimistic mutation be repeated?

[codazzo]

Hi! First of all thanks for your support so far 🙏

As I mentioned in some previous discussions (repeating here for reference) I'm trying to support a scenario whereby users of a native app would be able to create some "Recommendation" entities while the device is offline.
I'm using @urql/exchange-graphcache's offlineExchange, with an optimistic mutation and a custom updater which updates a query which lists recommendations

offlineExchange({
    storage, // <--- I created this using AsyncStorage
    schema,
    updates: {
        Mutation: {
            createRecommendation: (result, args, cache, info) => {
                const recommendation = .... // builds recommendation from args
                const fieldId = ... // gets field Id from args

                cache.updateQuery({ query: QUERY_RECOMMENDATIONS, variables: { fieldId } }, (data) => {
                    data.recommendations.push(recommendation);
                    return data;
                });
            },
        },
    },
    optimistic: {
        createRecommendation: ... // creates a Recommendation entry with a temporary ID
    }
});

Everything is working as expected except _maybe_ one thing which I though I would report.
Suppose the device is offline, and I create a Recommendation. The optimistic mutation runs, followed by the custom updater. After this QUERY_RECOMMENDATIONS has these entries

[
  { "id": "temporary_id_001" }
]

At some point the device goes back online, so the mutation that was saved as "pending" in the offlineExchange's queue gets re-executed. We again run the optimistic mutation and the custom updater. At this point QUERY_RECOMMENDATIONS has these entries

[
  { "id": "temporary_id_001" },
  { "id": "temporary_id_002" }
]

The mutation is successful as the device is online, so when a successful response is returned from the API the custom updater runs again. At this point QUERY_RECOMMENDATIONS has these entries

[
  { "id": "1144cee7-5d0d-4589-a26c-e1322e429b2d" }
]

For a very brief time (between when the second optimistic mutation is applied and when the response come back from the API) the query has two entries for the "createRecommendation" mutation, but the user only ever meant to create one.
What I see on a "Recommendations list" screen is a brief flickering between 1 reco, 2 reco, 1 reco when the device gets back online.

I implemented a workaround whereby I assign a random number to the mutation's payload and when the second optimistic mutation runs I check in the custom updater whether an entry already exists in the cache with that random number. If one exists, I return from the updater. This solves the issue with the flickering.

I nevertheless thought I would open this discussion as conceptually I'm not sure we should run the optimistic mutation twice as it would cause a brief inconsistent state unless the mutation is "idempotent". Would it make more sense to run the optimistic migration once when the device is offline, and then when the device gets back online we only apply the custom updater on the actual response from the API?

_Originally posted by @codazzo in https://github.com/FormidableLabs/urql/discussions/1078_

[kitten]

Hiya again 👋

So long story short; previously we prioritised a different case over this, which doesn't make much sense. There are three cases for repeated optimistic updates:

• Add something which appears in some kind of list
• Toggle something to flip its state
• Update its state

Unfortunately it's easy to tell that the first and second cases here stand in direct conflict. That is, if an identical mutation is fired twice we have to determine: Is it the same mutation that updates the list, or will it have different behaviour this time around? (e.g. toggling)

Previously we catered for the toggling behaviour. However that doesn't make much sense. Toggling is an odd one in an eventually consistent world and will only behave correctly if it's not concurrent anyway. So a UI can easily already prevent this from happening.

On the other hand we actually want to make the second case (_your case_) easier here. So instead of applying optimistic updates of an identical source mutation on top of eachother, we want to make them atomic.

You can test this fix in the PR above (#1080) with CodeSandbox CI (like last time) by setting Graphcache's version to: "https://pkg.csb.dev/FormidableLabs/urql/commit/0ddfd90a/@urql/exchange-graphcache"

[codazzo]

Again thanks a lot @kitten. Your explanation makes sense. I have tested the fix and can report as follows:

If I create one "reco" offline, and then I put the device online, I see this in the list (in chronological order)

[device offline]

[ { "id": "temporary_id_001" } ]

[device goes online]

[ { "id": "temporary_id_002" } ]

[response comes back from the server]

[ { "id": "1144cee7-5d0d-4589-a26c-e1322e429b2d" } ]

This does not cause any flickering as there's only ever one element in the list 👍

I then tried to create two more recommendations offline and it went like this

[device offline]

[
  { "id": "1144cee7-5d0d-4589-a26c-e1322e429b2d" },
  { "id": "temporary_id_003" },
  { "id": "temporary_id_004" }
]

[device goes online]

[
  { "id": "1144cee7-5d0d-4589-a26c-e1322e429b2d" },
  { "id": "temporary_id_003" },
  { "id": "temporary_id_004" },
  { "id": "temporary_id_005" },
  { "id": "temporary_id_006" },
]

[response comes back from the server]

[
  { "id": "1144cee7-5d0d-4589-a26c-e1322e429b2d" },
  { "id": "66bf05e7-e7b4-4280-b705-2904d38c0d29" },
  { "id": "6487fb5c-3576-44e5-8487-363d4f54a71e" },
]

This did cause some flickering when the device got back online.

Is there any other information I can provide? Unfortunately the only way I can test this is by running a production release on a real device (which means I can't access the JS dev tools). I can use console.logs though.

[kitten]

@codazzo oh yea, you just reminded me of why we disabled cumulative updates in the past. It’s exactly because of cumulative optimistic changes. I suppose we’ll need to find a bespoke mechanism for the offline exchange to clear all optimistic layers that it’ll affect before re-triggering the mutations.

To clarify, what’s going on here is that one of your optimistic updates builds on the other to add to the list. Then when they’re repeated, they update the list but the other optimistic updates aren’t cleared by then, which means that their own layer has been erased, but the other can still always see the changes.

[codazzo]

Thanks for the investigation @kitten.

For the record the "random number" trick I mention in the description works on the latest published version (no flickering whatsoever), whereas on 0ddfd90a I get something like

[device offline]

[
  { "id": "1144cee7-5d0d-4589-a26c-e1322e429b2d" },
  { "id": "temporary_id_003" },
  { "id": "temporary_id_004" }
]

[device goes online]

[
  { "id": "1144cee7-5d0d-4589-a26c-e1322e429b2d" },
  { "id": "temporary_id_005" },
]

[response comes back from the server]

[
  { "id": "1144cee7-5d0d-4589-a26c-e1322e429b2d" },
  { "id": "66bf05e7-e7b4-4280-b705-2904d38c0d29" },
  { "id": "6487fb5c-3576-44e5-8487-363d4f54a71e" },
]

Unfortunately screen recordings are all I have to go by since again I have no access to JS dev tools when testing the offline scenario.

I'm perfectly happy using the "random number" approach and wasn't even sure there would be a straightforward fix to this (or if it even was an issue really). I just thought I'd bring it to your attention.

[kitten]

Yea, so I'm basically think about several solutions, since it's not quite as straightforward as I thought. The advantage here is that we can always assume that optimistic layers are in order. The problem is that results are never in order and given that the order in which the second update is applied when the device comes back online is random, and clearing only their own layer isn't sufficient, so either:

• We need to clear all pending optimistic layers before retrying the mutations (This will need a new mechanism somehow)
• We need to be able to just store diff'ed link arrays in our data store (which is hard but which I'm willing to experiment with)
• We need to be able to find a way in which to apply the optimistic layers in the right order again when they're retried (This isn't probably possible since the cacheExchange has no power to reorder them, since they need to be executed synchronously but are pushed into the exchange, rather than pulled)

Obviously, just storing a diff of the list would be exceedingly cool 😆 But could introduce some non-trivial runtime cost.

[kitten]

@codazzo Ok, this isn't fully tested yet, but I pushed a temporary test of what it would look like to clear all the optimistic mutation layers first. Let me know if it even works or not 😅 but it should convey the basics of how it could function correctly, if it does work.

As last time, it's on CodeSandbox CI, version: "https://pkg.csb.dev/FormidableLabs/urql/commit/a20a0c81/@urql/exchange-graphcache"

[codazzo]

I have tested both cases in https://github.com/FormidableLabs/urql/issues/1079#issuecomment-713525552. I can report that in both cases the list refreshed exactly as I expected!

First case: creating one recommendation from none

[device offline]

[ { "id": "temporary_id_001" } ]

[device goes online]
[response comes back from the server]

[ { "id": "1144cee7-5d0d-4589-a26c-e1322e429b2d" } ]

Second case: creating two more recommendations starting from one

[device offline]

[
  { "id": "1144cee7-5d0d-4589-a26c-e1322e429b2d" },
  { "id": "temporary_id_003" },
  { "id": "temporary_id_004" }
]

[device goes online]
[response comes back from the server]

[
  { "id": "1144cee7-5d0d-4589-a26c-e1322e429b2d" },
  { "id": "66bf05e7-e7b4-4280-b705-2904d38c0d29" },
  { "id": "6487fb5c-3576-44e5-8487-363d4f54a71e" },
]

No flickering visible! Seems to work as far as I can tell :) Thanks @kitten!

[kitten]

Alright, awesome! Let's ship this then, and I'll talk to @JoviDeCroock to make sure that it's still a valid approach in the future, once we revisit this.

[kitten]

@codazzo Just one more thing; I did now revert the clearLayer change so that while we're thinking about this, any toggling behaviour still works. Just a minor detail, but basically this version should still work as is: "https://pkg.csb.dev/FormidableLabs/urql/commit/b8d0da50/@urql/exchange-graphcache"

[codazzo]

Just a minor detail, but basically this version should still work as is: "https://pkg.csb.dev/FormidableLabs/urql/commit/b8d0da50/@urql/exchange-graphcache"

Can confirm! The same tests yield the same results as in my previous message.

[kitten]

This has now been released 🙌🍀