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

Add proper query invalidation #2895

Closed
clayne11 opened this issue Jan 20, 2018 · 12 comments
Closed

Add proper query invalidation #2895

clayne11 opened this issue Jan 20, 2018 · 12 comments

Comments

@clayne11
Copy link
Contributor

clayne11 commented Jan 20, 2018

After investigating a pretty confusing performance problem in one of my apps, I came across a pretty serious issue.

As far as I can tell, every time any query or mutation occurs every single component that is watching any data in the apollo store gets re-rendered, even if none of its data has changed.

Imagine an e-commerce scenario: I favorite an item for later, which calls a mutation to save that favorited item on my account.

As a result, my entire page re-renders with hundreds of items since they're all being read from the apollo store, even though none of them have changed. This can take a few hundred milliseconds on a mobile device and cause stuttering and janking.

Intended outcome:
Each query's dependency graph is fully traversed and only updated if an object within that graph has changed.

Actual outcome:
Every single query is invalidated and re-run causing every component listening to any query in the store to re-render.

@Poincare
Copy link
Contributor

Is it possible for you to implement a shouldComponentUpdate on your components in order to determine whether or not your components should update?

The majority of the time isn't spent reading from the store according to the benchmarks, it's spent rendering that stuff on screen. So, if you're able to determine when to prevent renders correctly, that should solve a lot of the issue.

I remember we had worked on implementing a shouldComponentUpdate that just automatically did this for you, but maybe that got taken away due to other changes. @shadaj , @jbaxleyiii thoughts on this?

@fbartho
Copy link

fbartho commented Jan 30, 2018

@Poincare I think the feature-request is still pretty valid -- forcing every consuming component to implement a deep-equality shouldComponentUpdate is a pretty big operational burden, no? -- As far as I know, a shallow-equality-props shouldComponentUpdate doesn't work with Apollo, right?

Also, I'm looking at using react-apollo and the future recommended pattern is to use the Query component. Making sure it doesn't trigger when its normalized keys/values aren't updated seems like a valuable performance win, no?

@clayne11
Copy link
Contributor Author

clayne11 commented Jan 30, 2018

I definitely think it's valid feature request. Implementing shouldComponentUpdate is definitely not the way to solve this problem IMO. It's very error prone and puts a huge burden on application developers when the issue can be solved at the library level.

Query invalidation is not a trivial feature to implement but it's absolutely critical for large apps. I'm starting to have serious performance problems when unrelated queries or mutations are causing my entire UI to re-render.

I'm pretty confident that it's possible to implement query invalidation in a performant way by tracking the dependency graph of each query.

I don't think that the solution is to implement shouldComponentUpdate in react-apollo either. I strongly believe that proper query invalidation should be built into the core of apollo-client (or maybe in the apollo-cache-inmemory, I don't know the architecture well enough to say for sure).

@Poincare
Copy link
Contributor

Poincare commented Feb 3, 2018

Could you detail how exactly the query invalidation logic might work? For example, let's say you have the following query already on the page:

query {
  author(idStart:0, idEnd:10) {
     name
  }
}

And you fire off a mutation that adds a new author but this author has an id of 14 and so wouldn't be a part of the resultset for this query. You can also imagine cases where a mutation affects queries that don't explicitly have the same structure but request overlapping data. In cases like this, how would we construct the dependency graph for a mutation or query?

@clayne11
Copy link
Contributor Author

clayne11 commented Feb 6, 2018

Right now that case isn't handled anyways. When you have a list query, new items won't show up into that list unless you either have a subscription, refetch the query (using refetchQueries on the mutation), or manually write to the store. apollo-client has no way to know that it should be adding that author to the result of the query since the logic for which authors are part of that query come from a resolver on the server.

Even though the mutation to add an author will cause any item observing that list query to re-render, the new author with ID 14 won't be rendered into the list, so it will be a completely wasted render.

Instead, I'm proposing that unless the result of


query {
  author(idStart:0, idEnd:10) {
     name
  }
}

actually changes, through one of the techniques mentioned above, we shouldn't be broadcasting a change to this query because there isn't one.

Did you want me to go into a deeper proposal on how the query invalidation actually works from a technical / implementation perspective or does that make sense?

@darren128
Copy link

darren128 commented Feb 21, 2018

This is also an issue for us. We have some large queries with many components using fragments of the larger query.

Actual
When performing a fetch for the fragments, the larger query's cache is invalidated simply because it contains the smaller query fragment. This causes the observable queries to be fired unnecessarily leading to excess rendering. Our only workaround at the moment is to do a deep equality check on our own to decide if the component should rerender or not.

Expected
Observable queries (their cached data) should not be invalidated if the actual data has not changed.

@Poincare
Copy link
Contributor

Poincare commented Feb 22, 2018

@clayne11

I believe that the component should not re-render if the previous result for the associated queries has not changed. Would it be possible for you to produce a reproduction of this issue?

@clayne11
Copy link
Contributor Author

clayne11 commented May 4, 2018

@Poincare the previous result will always change after any operation that writes to the store with the current architecture because every read returns a new object so previousResult !== newResult even if nothing has changed.

It looks like this PR will solve the issue I'm describing.

@niieani
Copy link
Contributor

niieani commented May 10, 2018

We have this (and some other perks) in our alternative implementation, leveraging apollo-link + apollo-cache-inmemory.

We'd love to merge efforts, but probably we'd need to consult with repo members (CC @jbaxleyiii) before any PRs, cause our changes are pretty huge (even though the API is mostly the same).

@clayne11
Copy link
Contributor Author

clayne11 commented May 10, 2018

@niieani can you link to that solution? I'd like to check it out.

@niieani
Copy link
Contributor

niieani commented May 10, 2018

@clayne11 It's not OSS yet. We want to make it public, but currently lacks docs and has some dependencies on our private monorepo, which I need to untangle.

@hwillson
Copy link
Member

hwillson commented Aug 5, 2018

To help provide a more clear separation between feature requests / discussions and bugs, and to help clean up the feature request / discussion backlog, Apollo Client feature requests / discussions are now being managed under the https://github.com/apollographql/apollo-feature-requests repository.

This feature request / discussion will be closed here, but anyone interested in migrating this issue to the new repository (to make sure it stays active), can click here to start the migration process. This manual migration process is intended to help identify which of the older feature requests are still considered to be of value to the community. Thanks!

@hwillson hwillson closed this as completed Aug 5, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 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

6 participants