Apollo-client: Cache API design

[jbaxleyiii]

This issue is an umbrella issue for tracking discussions related to the new store API design and store's built with it!

cc @shadaj @nevir

[nevir]

Some scattered feedback as I dig into the code:

1. It would be nice if the cache was a pure interface, so we didn't have to set up a circular dependency on apollo-client. I think there's a couple changes needed for this:

   • Reduce the surface area of the Cache's interface. Probably: drop readQuery/readFragment (in favor of read) from the API, and instead make them pure functional helpers that AC uses internally. Ditto write.

   • Can diffQuery be folded into read? It overlaps with returnPartialData (and it's not great that we're using exceptions as control flow in that case).

2. addTypename smells like a specific requirement of the InMemoryStore, but not something generic to all caches (say, ones that pre-process the schema, or have servers describe theirs)

3. I'm not entirely clear on when/why AC asks for non-optimistic (raw) data from the cache; have any clarity or pointers for me?

4. I'm wondering if maybeDeepFreeze should maybe be a concern of the cache? Smells weird where it's at right now. (on the flip side, it's nice where it's at - I don't have to re-implement it)

[stubailo]

Link to PR for context: #1921

[stubailo]

I'm not entirely clear on when/why AC asks for non-optimistic (raw) data from the cache; have any clarity or pointers for me?

You want this if you are planning to update the non-optimistic data with a readQuery/writeQuery combo.

[nevir]

You want this if you are planning to update the non-optimistic data with a readQuery/writeQuery combo.

Unless I'm missing something, there's no _public_ API from AC that allows someone to specify optimistic: false, right?

Calls within AC that I'm unclear on:

---

QueryManager#fetchQuery calls diffQuery({ ... optimistic: false }), which is surprising to me. If the optimistic view of the data satisfies the query (and it's not network-only), wouldn't we want it to emit a value? Also, it appears like the result from this call is passed directly to the caller without having optimistic data layered on. Similarly, it calls through to QueryManager#fetchRequest, which follows suit.

I read that as fetchQuery/fetchRequest being concerned with only the server's view of the data (makes sense), but does it seem to imply that AC has to then re-query for (or layer on) optimistic values before returning via its public API (which it does not seem to do). I think I'm missing something.

---

DataStore#markMutationResult's use of optimistic: false for updateQueries seems clear - check any updateQueries if present, and only write them to the cache if values have changed. However, I'd like to argue that this is something that the cache should perform for _all_ writes (and that AC can skip this particular check)

---

(Those are also, as best I can tell, the only call sites that ask for non-optimistic data)

[Fxlr8]

Ability to read/update instances in store using data id only would be very useful

[nevir]

@Fxlr8 I believe you can do that today via readFragment

[Fxlr8]

@nevir, readFragment as API reference says, requires me to:

1. write/import a fragment
2. add fragment name
3. add fragment-specific variables

it is unwanted stuff to do when I just want to read/change a single object in the store. data id is unique for the whole store and is enough to identify and get a single item

[Fxlr8]

Also, there is a problem when I want to readQuery on a query with pagination. Here is a simple example to show you the problem. Imagine this data structure:

user {
    id
    name
    friends(state: friendshipState, after: Cursor, count: Int ){
        edges {
            node {
               friend {
                   state
                   user {
                   ...
                   }
               }
        }
    }
}

edges and node is just relay-styled pagination . Then in a view i make two queries:

user {
    friends(state: "INCOMING" ...pagination stuff here )
}

user {
    friends(state: "ACCEPTED" ...pagination stuff here )
}

and I have an endless scroll on my page, so when the user scrolls to the bottom of one of the lists, that list's query is repeated, but with different pagination params, so that new items are loaded. Just like in the docs here

The problem starts here. Imagine that user scrolled 'incoming' friends list and a couple of pagination queries fired. Then the user clicks 'accept friend' button on one of the requests. A mutation is fired, and now I need to make a cache update and move friend's record from 'incoming' list to 'accepted'. What query should I use in readQuery in this case if I had three queries with different pagination params to collect 'incoming' friends list? I tried to use a query from above, I tried to add query variables, but I get an error:

errorHandling.js:7 Error: Can't find field friends({"state":"INCOMING"}) on object (User251586469) {
"id": "251586469",
},
"friends({\"state\":\"INCOMING\",\"first\":30})": {
"type": "id",
"id": "$User251586469.friends({\"state\":\"INCOMING\",\"first\":30})",
"generated": true
},
"friends({\"online\":true,\"first\":30})": {
"type": "id",
"id": "$User251586469.friends({\"online\":true,\"first\":30})",
"generated": true
},
}

I can't provide correct pagination params to readQuery because there is no easy way to distinguish which item relates to which page of pagination. I wish there would be a better way of managing objects and their relations in the cache. I don't know which exactly, here is just a use case, where current store API becomes sophisticated. Or is it just me doing it wrong?

[nevir]

Ah, yeah - that makes sense. I can't speak to the pagination bit (I think it might get merged into a single list somewhere in the cace)

As for fetching state from Apollo's current cache without a fragment - unless you only want shallow values (e.g. no nested objects/arrays), it's a no-go. The cache needs to know how far to walk (and reconstruct) your nodes to return them. Alternate caches, once we get this interface going, should be able to provide what you're looking for, though

[nevir]

Feedback on recordOptimisticTransaction:

The current behavior appears to be to capture the transaction callback, and (re-)call it each time the store wants to (re-)apply the optimistic update. But for situations where the app must be shut down (and the cache needs to be marshalled/restored), that's not going to work (at least at this level).

What I'm thinking of doing is:

• Record all writes (as POJOs), which can be applied when replaying optimistic updates
• _Disallow_ reads from the transactional Cache - possibly entirely, or more likely only reads that occur after a write. (To avoid cases where people are relying on the values they just wrote)
  
  
  
  • Or, alternatively, reads could read from the state of the store prior to the transaction beginning (even after a write). But that seems like it'd be pretty confusing
  
  

---

Edit: I've circled back to just capturing the update function for now; and am ignoring offline support. I think we need a different sort of public API to support that better. Maybe.

[nevir]

Another point:

• Transactions should probably be given a DataProxy, not an entire Cache object (hard to reason about calling reset, watch, removeOptimistic, etc within a transaction - also would be nice to ignore nested transactions)

[stubailo]

@Fxlr8 you'll be interested to read about the @connection directive, which is exactly for that purpose - to make more predictable store keys for complex arguments: http://dev.apollodata.com/react/pagination.html#connection-directive

Transactions should probably be given a DataProxy, not an entire Cache object

👍 - I agree that the methods should be limited to the ones that make sense

The current behavior appears to be to capture the transaction callback, and (re-)call it each time the store wants to (re-)apply the optimistic update.

I think that is an implementation detail of InMemoryCache - other implementations could do it differently, like storing a set of patches on the data.

it is unwanted stuff to do when I just want to read/change a single object in the store.

The problem here is field arguments. Since JS has no concept of a field with arguments, you kind of need the fragment to specify which arguments you're trying to read:

fragment Price on InventoryItem {
  price(location: USA)
}

# vs.
fragment Price on InventoryItem {
  price(location: AUS)
}

So it's not really clear what a "single object" in GraphQL should look like. Besides, different store implementations might store the objects in different ways.

I think this might be a place where store implementations can provide non-standard APIs like getObject(id), which will do different things depending on the underlying implementation.

[nevir]

Looks like there's a couple straggling spots where addTypename is still managed outside of the cache (spotted by @vincecoppola). Would be good to have consistent ownership for that config flag:

• https://github.com/apollographql/apollo-client/blob/2.0-alpha/packages/apollo-client/src/core/QueryManager.ts#L355
• https://github.com/apollographql/apollo-client/blob/2.0-alpha/packages/apollo-client/src/core/QueryManager.ts#L1093

[nevir]

I'm definitely struggling to understand when the cache should throw exceptions vs not - for example https://github.com/convoyinc/apollo-cache-hermes/pull/32 - from reading the in memory cache code, it seems like diffQuery _should_ be throwing if returnPartialData is falsy (aka the default), but it doesn't appear to. And things like query observers don't appear to expect it to throw

[jbaxleyiii]

@nevir I was just talking with @shadaj about the typename issue! Working on a solution for sure!

hmm, I'm not sure on throwing. We should set policies for sure there. Maybe even adjust to not throw anywhere and have error responses somehow? I don't like throwing when possible

What are your thoughts?

[nevir]

IMO the cache shouldn't throw at all - but instead return isMissing when
requested. That way AC can choose what to do based on it

On Wed, Aug 9, 2017, 08:58 James Baxley [e-mail] wrote:

@nevir https://github.com/nevir I was just talking with @shadaj
https://github.com/shadaj about the typename issue! Working on a
solution for sure!

hmm, I'm not sure on throwing. We should set policies for sure there.
Maybe even adjust to not throw anywhere and have error responses somehow?

What are your thoughts?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/apollographql/apollo-client/issues/1971#issuecomment-321299767,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAChnacAUrB0Uj8XAMxRitC90dHEF1FJks5sWdcNgaJpZM4OpGfu
.

[nevir]

Aha, so I missed that returnPartialData _defaults_ to true (might want to flip that option around in the cache API to make it more apparent, and less of a landmine)

(throwOnMissing or something)

[nevir]

Oof, more gas to douse on to the "don't expect + trap errors coming from the cache" fire: just lots a lot of time to a genuine bug in my cache that was being swallowed by AC, and treated as a partial result

[jvbianchi]

is there a design doc with the new planned Cache API?

[Fxlr8]

By the way, why do you guys want to drop the query reducer feature?

[shadaj]

@Fxlr8 Before, Apollo Client always had a single cache implementation that had a structure that could be updated with a reducer. With the new Cache API, there can be cache implementations that do not have an in-memory representation that can be updated with a reducer, so we decided to remove the feature so that Apollo Client could be more flexible.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions to Apollo Client!

[cport1]

Doesn't version 2.0 pretty much take care of this issue?

[jbaxleyiii]

I'm going to close this out in favor of a new issue to formally document the API. cc @shadaj would you be up for documenting it?