unnecessary updates when mutations are issued in rapid succession

[akiran]

We have a live edit feature where we send mutation request on every key change.
We noticed a issue when we type fast and send mutation requests in rapid succession

Lets say we type 4 characters very fast. update method passed to mutation should be executed 8 times (4 times for optimistic response and 4 times for actual mutation response). But we noticed that update method is executed more than 8 times. I think, this is causing us a performance issue.

I replicated the issue in this repo
https://github.com/akiran/apollo-mutation-issue

This demo has an input field which send mutation on every key change. I added 10 sec delay on server to delay mutation response.
When I type 4 character quickly (let say "john"), 4 optimistic updates will execute first because of 10 second delay on mutation response. Then mutation response update start happening.

In ideal scenario, only 1 update related to mutation response should be executed because response from previous requests can be ignored and update for only final response can be executed

But because of some bug, 10 updates are executed (4 optimistic and 6 mutation response updates)

Let me know if more information is required related to this issue

Versions
System:
OS: macOS High Sierra 10.13.6
Binaries:
Node: 8.12.0 - ~/.nvm/versions/node/v8.12.0/bin/node
Yarn: 1.10.1 - ~/.nvm/versions/node/v8.12.0/bin/yarn
npm: 6.4.1 - ~/.nvm/versions/node/v8.12.0/bin/npm
Browsers:
Chrome: 70.0.3538.77
Firefox: 60.0.1
Safari: 12.0
npmPackages:
apollo-boost: ^0.1.19 => 0.1.19
apollo-server: ^2.1.0 => 2.1.0
react-apollo: ^2.2.4 => 2.2.4

[benjamn]

Thanks for the reproduction! When I run the app with npm start, it starts up at localhost:3000, but it looks like it's trying to connect to a server at localhost:4000. How should I start that server?

[akiran]

@benjamn You can start server with npm run server

[benjamn]

Ok, I'm fairly certain this behavior was introduced by my commit d6a673f, which removed the behavior of silencing broadcasts when previousResult === newData.result, because that's not a safe way to tell whether the data have changed, so it's better to broadcast the data than to silence the broadcast.

In this specific case, the new behavior means that this.broadcastWatches() in the InMemoryCache#removeOptimistic method now actually broadcasts watches, even if the optimistic data have not changed.

I'm not sure how best to fix this, but it would probably break #3992 again if we went back to silencing these broadcasts, so that's not really an option.

[joe-re]

@benjamn
I might have misunderstood, but I feel we should keep object mutability when update caching object.
Specifically, I think, we should use items = items.concat[addedItem] instead of items.push(item).
That assumption is the same on react, args of this.setState should keep mutability.
So I feel, I prefer previous working to currently.

[hwillson]

This should no longer be an issue with current versions of apollo-client. Thanks!