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

(core) - Make Client more forgiving by sharing result sources that emit cached results #1515

Merged
merged 7 commits into from
Apr 28, 2021

Conversation

kitten
Copy link
Member

@kitten kitten commented Apr 4, 2021

Summary

At the risk of being called edgy, this proposed change layers a third cache on top of the Client. Apart from the usual exchange cache (default cacheExchange or Graphcache) and the React bindings' suspense/concurrent cache, this third cache is just a minor one to share results across all sources.

This change replaces the Client's semaphore with a shared binary semaphore that only stores the result source for a given operation. Hence, an active operation now has a single source that it uses for all subscribers. Apart from the shared source on each subscription it will still emit a new operation. However, all sources for a single subscription now share an underlying source. This source ensures that all operations see the exact same result (which was only implicitly guaranteed before) and each new subscriber will receive the last previously known result if its operation doesn't immediately yield a first result.

This means that it'll be harder to trip up when layering a lot of logic with shared operations that have multiple concurrent subscribers and multiple concurrent uses of cache-and-network request policies.

Set of changes

  • Replace activeOperations type with a Map<number, ActiveOperation>
  • Store a result source for each query/subscription source that's shared and cleans itself up
  • Keep around an "active cached value" for each source that's the last known result
  • Clear the activeOperations when it ends
  • Use the "active cached value" to emit a first value for new subscribers when they start and don't receive a synchronous result immediately
  • Ensure that each new subscriber issues a fresh operation

The risk associated with this change right now is that the requestPolicy may be disregarded. We have to investigate whether a network-only operation for instance still shows the expected bindings behaviour. Although, this hasn't been doing what people thought anyway.

@kitten kitten force-pushed the feat/single-source-behavior branch from 8a23d5d to 4538812 Compare April 4, 2021 14:51
@changeset-bot
Copy link

changeset-bot bot commented Apr 4, 2021

⚠️ No Changeset found

Latest commit: 8eb7909

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

kitten added 3 commits April 22, 2021 13:21
Replace the client's counting of active operations with a
single, shared operation result source in a Map. This achieves
the same semaphore behaviour but replaces it with a binary
semaphore-like cache, where the cached source remembers the
last active result.
@kitten kitten force-pushed the feat/single-source-behavior branch from 607869b to bca99c7 Compare April 22, 2021 12:21
@kitten kitten marked this pull request as ready for review April 22, 2021 13:37
@kitten
Copy link
Member Author

kitten commented Apr 22, 2021

I'd say from a "proof of concept" perspective, this is ready and working now. I'm not quite happy with the implementation in Client yet, but the tests should be able to guide us to a better implementation as possible and needed. The difficulty here is that we'd like result sources not to replay prior results if the cache offers a result up immediately/synchronously, which makes encapsulating this logic difficult, as the cache would emit a result in onStart which makes this less composable. So either it won't be any prettier/elegant or we can change the timing of this a little.

We also need to test whether this behaviour actually improves the intuitiveness of cache-and-network and network-only in some apps. It does make network-only behave a little more like cache-and-network, so we may want to instead ignore those as needed and not replay results if network-only is used. The question is whether the stale: true behaviour is a little too aggressive in that case but we have an existing test case for that too, so we can adjust as needed. My hunch is that network-only should not replay any results.

@kitten kitten force-pushed the feat/single-source-behavior branch from 7018a49 to 7fd4474 Compare April 22, 2021 13:42
packages/core/src/client.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants