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

[Regression] apollo-client 3.0.0-beta.44 does not rollback optimistic updates #6183

Closed
darkbasic opened this issue Apr 22, 2020 · 5 comments
Closed
Assignees
Milestone

Comments

@darkbasic
Copy link

darkbasic commented Apr 22, 2020

This is the repro: https://github.com/darkbasic/gh6183

After cloning use yarn && yarn start to run it. There is a button next to each article: if you delete the first two of them everything will work out as expected, but as soon as you will delete the third one the server will return an hardcoded error after a 3 seconds delay and apollo-client won't rollback the optimistic update.
If you run git revert HEAD && yarn install:clean && yarn start you will downgrade to apollo-client 2.6 and you will find that everything will work as expected: after 3 seconds apollo-client will rollback the failed mutation.

Versions

  System:
    OS: Linux 5.6 Arch Linux
  Binaries:
    Node: 13.13.0 - ~/.nvm/versions/node/v13.13.0/bin/node
    Yarn: 1.22.4 - /usr/bin/yarn
    npm: 6.14.4 - ~/.nvm/versions/node/v13.13.0/bin/npm
  Browsers:
    Firefox: 75.0
  npmPackages:
    @apollo/client: ^3.0.0-beta.44 => 3.0.0-beta.44
@darkbasic
Copy link
Author

I suspect that re-accessing the same query with another useQuery will provide the correct result, thus the bug is probably just active queries not being updated. Neverthless this is not expected behaviour and a regression compared to apollo-client 2.6.

@benjamn benjamn added this to the Release 3.0 milestone Apr 22, 2020
@castiedemann
Copy link

#5790

@3nvi
Copy link

3nvi commented Apr 27, 2020

Yeah I think this is a duplicate of #5790

@benjamn benjamn self-assigned this Apr 28, 2020
@darkbasic
Copy link
Author

darkbasic commented May 18, 2020

I gave this another try with @beta.48 (#6221), but unfortunately it still doesn't roll it back.

Worth mentioning, I also noticed another bad behaviour on the Apollo side: if I use cache.evict to delete the article (instead of writeQuery) it doesn't wait for the mutation response before re-fetching. Instead, as soon as it gets the optimistic reponse it immediately refetches from the server, which may or may not hold the correct response at this point. If you feel like this is worth its own bug report let me know and I'll open another one.

@hwillson
Copy link
Member

hwillson commented Jun 1, 2020

We're looking into this and tracking progress in #5790, so I'll close it here. Thanks!

@hwillson hwillson closed this as completed Jun 1, 2020
benjamn added a commit that referenced this issue Jun 9, 2020
This fixes an issue found by @darkbasic while working with optimistic
updates: #6183 (comment)

The cache-first FetchPolicy is important not just because it's the default
policy, but also because both cache-and-network and network-only turn into
cache-first after the first network request (#6353).

Once the cache-first policy is in effect for a query, any changes to the
cache that cause the query to begin reading incomplete data will generally
trigger a network request.

However, if the source of the changes is an optimistic update for a
mutation, it seems reasonable to avoid the network request during the
mutation, since there's a good chance the incompleteness of the optimistic
data is only temporary, and the client might read a complete result after
the optimistic update is rolled back, removing the need to do a network
request.

Of course, if the non-optimistic read following the rollback is
incomplete, a network request will be triggered, so skipping the network
request during optimistic updates does not mean ignoring legitimate
incompleteness forever.

Note: we already avoid sending network requests for queries that are
currently in flight, but in this case it's the mutation that's in flight,
so this commit introduces a way to prevent other affected queries (which
are not currently in flight, themselves) from hitting the network.
benjamn added a commit that referenced this issue Jun 10, 2020
This fixes an issue found by @darkbasic while working with optimistic
updates: #6183 (comment)

The cache-first FetchPolicy is important not just because it's the default
policy, but also because both cache-and-network and network-only turn into
cache-first after the first network request (#6353).

Once the cache-first policy is in effect for a query, any changes to the
cache that cause the query to begin reading incomplete data will generally
trigger a network request, thanks to (#6221).

However, if the source of the changes is an optimistic update for a
mutation, it seems reasonable to avoid the network request during the
mutation, since there's a good chance the incompleteness of the optimistic
data is only temporary, and the client might read a complete result after
the optimistic update is rolled back, removing the need to do a network
request.

Of course, if the non-optimistic read following the rollback is
incomplete, a network request will be triggered, so skipping the network
request during optimistic updates does not mean ignoring legitimate
incompleteness forever.

Note: we already avoid sending network requests for queries that are
currently in flight, but in this case it's the mutation that's in flight,
so this commit introduces a way to prevent other affected queries (which
are not currently in flight, themselves) from hitting the network.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants