Optimistic mutations should only update store once if optimistic result was correct

[wmertens]

TL;DR:

• Optimistic mutations should only update cache if the server result differs from the optimistic result - except if the mutation is marked pessimistic
• All in-flight mutations should be marked pessimistic when a mutation result differs from the optimistic one

Intended outcome:

Increase a counter via optimistic UI. Every click instantly shows the new value, and there is no glitching as server results come back.

Actual outcome:

Mutations always update the store with the result, so the counter will glitch back to the server result.

How to reproduce the issue:

schema:

  type Query {
    count: Int
  }
  type Mutation {
    inc: Int
  }

The mutation for inc calls mutate({optimisticResponse: count+1}) (plus the function to update the count in the cache).

Here's what count will be when you click it once:

click     |==mutate==1|

count  0  1           1

and repeatedly:

click     |==mutate==1|
click        |===mutate==2|
click           |==mutate==3|
click              |==mutate==4|
click                   |==mutate==5|

count  0  1  2  3  4  1 2 2 3  4    5

The solution is that the mutation should check if the response from the server is equal to the optimisticResponse, and if so, leave the store unchanged. This would result in this:

click     |==mutate==1|
click        |===mutate==2|
click           |==mutate==3|
click              |==mutate==4|
click                   |==mutate==2|

count  0  1  2  3  4    5           

If somewhere along the way the server decides the answer is different, you get

click     |==mutate==9|   (optimistic 1)
click        |===mutate=10|   (optimistic 2)
click           |==mutate=11|   (optimistic 3)
click              |==mutate=12|   (optimistic 4)
click                   |==mutate==13|   (optimistic 10)

count  0  1  2  3  4  9  10 11 12   13

So in this case, there are jumps, but never backwards

Note that if the server would not use inc but set, it messes up the final value (should be 5):

click     |==mutate==1|
click        |===mutate==2|
click           |==mutate==3|
click              |==mutate==4|
click                   |==mutate==2|

count  0  1  2  3  4  1 2 2 3  4    2

[joshribakoff]

There seems to be a similar discussion for relay here that may be of interest. facebook/relay#2481

[wmertens]

yes, and luckily apollo is one step further along for implementing it :) let's

…

On Fri, Jul 20, 2018, 5:01 AM Josh Ribakoff ***@***.***> wrote: There seems to be a similar discussion for relay here that may be of interest. facebook/relay#2481 <facebook/relay#2481> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3691 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADWlhhauH6dizUOUyt66hkGqk9VaxOKks5uIUgfgaJpZM4VQMkM> .

[joshribakoff]

What about failures? If I send 5 increments and and 5 are in flight, then the first one fails, then what happens? Should it rollback the other 4 optimistic responses? They would all be wrong (off by one).

Also I think I found a pathological case where your proposed fix would cause issues:

click     |==mutate==1|   (optimistic 1 matches)
click        |===mutate=3|   (optimistic 2 differs)
click           |==mutate=4|   (optimistic 3 differs)
click              |==mutate=5|   (optimistic 5 matches)

count  0  1  2  3  5  x  3 4  x

The count appears to revert from 5 to 3, going backwards... Additionally we'd show 4 in the UI at the end of this sequence instead of showing 5, which is wrong.

Instead maybe we can do this...

click     |==mutate==1|   (optimistic 1 matches)
click        |===mutate=3|   (optimistic 2 differs)
click           |==mutate=4|   (optimistic 3 differs)
click              |==mutate=5|   (optimistic 5 matches)

count  0  1  2  3  4  x  x x  5

If at any time there are multiple in flight mutations and a mutation's actual response is received and it differs from its corresponding optimistic response, then we rollback all other optimistic updates and transition the query to pessimist mode (prevent any new optimistic updates), until all requests settle at which point we switch back to optimist mode and use the latest value.

Or in other words if at any time things differ, we just block all updates until all requests settle, avoiding superfluous jumps. You don't know what data structure is being updated or any of its properties. It may not be a contiguous linear counter. It may be subject to complex business logic on the server.

Another approach is to let users specify an immutable update method, and substitute previous optimistic values with their correct values as they arrive, and continually re-run all pending operations' immutable reducers to compute a new optimistic state. This would require a breaking API change unless it was optional / opt-in only.

Note that if the server would not use inc but set, it messes up the final value (should be 5)

Thats basically operations vs. state updates...

For all mutations, it could use the "pessimest mode" algorithm and toggle between pessimistic / optimistic mode. Once all in flight requests settle, it would always switch back to optimist mode.

For the subset of mutations that are state updates - The apollo-link package uses observables. In rxjs you'd switchMap to handle this race condition in one line of code. @evans has a PR open to switch apollo-link from zen to rx. switchMap would basically cancel any in flight mutations and issue a new request with the correct value. This would not only solve for race conditions it would also save network requests / give a performance boost for when there is a state update and the user triggers multiple concurrent mutations. Only the last mutation need be attempted. This in effect prevents concurrent requests, so it co-exists fine with "pessimist mode"

We should also update the docs. It should be clear to people reading the documentation how this all works exactly. A counter is a rather basic example and right now its not clear in the docs how this works. That could deter people from using apollo, or worse people could write code with the wrong assumptions in mind. It should be clear in the docs what assumptions can be made about how the state changes over time and in corner cases not just the happy path.

[wmertens]

@joshribakoff Oops missed this.

If the server does not send the expected optimistic response, then the cache should instead show the server response.

The pathological case you show is indeed a problem - to summarize, a bunch of mutations are sent, a couple of them fail resulting in an updated cache but the final mutation has the expected result, so it won't put that new result in the cache.

This seems like an edge case, so your "pessimistic mode" proposal looks ok. I think the safest way to implement it is to mark all in-flight mutations as pessimistic, not just the ones for that query.

I'm not sure about the other things you propose, I don't use the apollo state stuff. Note thought that in-flight mutations that already departed should never be cancelled, since you don't know if the network request already went to the server.

So to summarize:

• Optimistic mutations should only update cache if the server result differs from the optimistic result - except if the mutation is marked pessimistic
• All in-flight mutations should be marked pessimistic when a mutation result differs

[elimydlarz]

Any word on this from the team?

It seems like a necessity if we want apps that are tolerant of intermittent connectivity,.

At the moment, users will watch their offline, already-optimistically-rendered changes get rewound and replayed when they regain connectivity. That's a pretty terrible experience if they were in the middle of doing something.

@wmertens suggestion sounds extremely reasonable and useful.

[hwillson]

Please try a recent version of @apollo/client and let us know if this issue is still happening. Thanks!

[joshribakoff]

2 years for a disappointing response. Apollo's lack of attention to detail on these types of edge cases make me not want to use the library on future projects, if I have a choice.

[hwillson]

@joshribakoff Have you tried @apollo/client? We've addressed almost all of the outstanding issues in this repo, including changes to how optimistic responses work. While I agree the communication on this issue and others has been sorely lacking, that's caused entirely by the fact we're a very small team, and are focused on pushing Apollo Client development forward (which unfortunately means we don't have as much time to triage issues as we'd like). Please give @apollo/client a shot, and post back if you still think there is an issue here.

[joshribakoff]

@hwillson no because I no longer work at Twitch where we used it I work in the autonomous vehicle industry now and we don’t use gql.

I empathize, but please understand I tried to be a contributor and help your small team but PRs sat, issues lingered, and Apollo could do a better job of working together with he community. If it were my lib, I would personally verify if the issue was fixed because I’d want my lib to be high quality and not blow up at runtime on people, which was something that has occurred in the past when using Apollo, and when trying to clarify these edge cases Apollo has been silent historically

[hwillson]

For sure @joshribakoff - all valid points. Thanks for the feedback!