apollographql / apollo-feature-requests

πŸ§‘β€πŸš€ Apollo Client Feature Requests | (no πŸ› please).
Other
130 stars 7 forks source link

In-flight deduplication does not behave the same as cache #381

Open chughes87 opened 2 years ago

chughes87 commented 2 years ago

Intended outcome: Two, or more, in-flight requests for the same resource should be deduplicated as long as the queries after the first request a subset of the first query's data.

Actual outcome: If two, or more, queries differ at all, the requests will not be deduplicated even if they request the exact same resource and the cache would register a cache hit if the requests execute serially.

How to reproduce the issue: I've created a simple repository that clearly reproduces the issue. The initial commit shows the deduplication failing to dedupe similar queries: https://github.com/chughes87/apollo-bug-repro/commit/05ef27abcf0918f8e149c1bcd1c469cdb6ff0980. The latest commit shows the cache successfully de-duplicating the second request: https://github.com/chughes87/apollo-bug-repro/commit/686a3d964032f7087582d31ac456165830e204ad

Versions System: OS: macOS 12.6 Binaries: Node: 18.9.0 - /opt/homebrew/bin/node Yarn: 1.22.19 - /opt/homebrew/bin/yarn npm: 8.19.1 - /opt/homebrew/bin/npm Browsers: Chrome: 106.0.5249.61 Safari: 16.0 npmPackages: @apollo/client: ^3.6.9 => 3.6.9

chughes87 commented 2 years ago

This may be what makes others complain about https://github.com/apollographql/apollo-client/issues/956

chughes87 commented 2 years ago

I'm interested in implementing this feature. If someone could provide some pointers on how to get started I'd appreciate it!

chughes87 commented 2 years ago

Based on what I've seen in the codebase, it seems that the inFlightLinkObservables part here: https://github.com/apollographql/apollo-client/blob/main/src/core/QueryManager.ts#L978 is what needs updating. Currently, simply storing the queries in a Map as the key values and using .get to establish a "cache hit" in this case makes it so that only queries that match exactly will result in deduplication. It seems that there exists logic in Apollo Client to perform deep comparisons between queries in the cache system to determine if there is a cache hit or miss. I've tried to find the code that does this part but wasn't successful last night. If there was a method that I could use that I could supply two queries to that would tell me if the second is a strict subset or equivalent, I could easily update this code to behave as I expect it to. I was also considering simply writing a recursive algorithm that would compare two query objects. That would not be difficult to implement, but I fear I'd be reinventing the wheel

chughes87 commented 2 years ago

Looking more broadly at the codebase today. Seems like I could possibly make use of src/utilities/common/mergeDeep.ts for this purpose. I could merge the existing query with the incoming one. If the result is equal to the existing query, the second one is a strict subset of the first.

chughes87 commented 2 years ago

Cool! I have an initial proof of concept here: https://github.com/apollographql/apollo-client/compare/main...chughes87:apollo-client:improve-in-flight-dedup. Still have to figure out the cleanup code that I just commented out. Maybe there is a better data-structure to use than Array?

Edit: Here's a draft PR so that people can leave comments: https://github.com/apollographql/apollo-client/pull/10163. I think I may want to add more testing when I have time.

jpvajda commented 2 years ago

@chughes87 Thanks for the contribution here! We appreciate it! the team is going to prioritize taking a look at your PR very soon. πŸ™

vpelletierupgrade commented 1 year ago

Came here from #956 Still have problems with queryDeduplication: false. The cache is not the issue. Is this issue stale?

chughes87 commented 1 year ago

@vpelletierupgrade as far as I know, this is still an active issue. There is a deduplication feature, but it does not behave as the cache does and simply does a check for if the queries coming in are exactly the same.

chughes87 commented 1 year ago

Oh also I didn't update here. The contribution attempt I made above was not successful. I found that my approach was flawed because it was dependent on being able to do deep comparisons on two different objects that potentially had arrays in them that would have the same information but that information would be in different orders. So the comparison failed. I was unsure on how to adjust the comparison logic to account for that. At least that is what I remember. It was a few months ago.

jerelmiller commented 1 year ago

@chughes87 good memory πŸ˜„.

For context, here is the PR: https://github.com/apollographql/apollo-client/pull/10163

siddhant928 commented 1 year ago

@chughes87 Did you find any workaround for this flow to work till the deduplication issue is fixed? Any good way to stop these duplicate requests?