Expose a way to read an object given an id

[Poincare]

Exposes a method on ApolloClient that allows you to read an entire object given its id without worrying about how that object has been normalized within the store. See #474 for some reasoning behind this.

TODO:

• Update CHANGELOG.md with your change
• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass

[nevir]

It would probably be helpful to expose the underlying readObjectByIdFromStore as well, so that we can consume it via react-apollo or react-redux (e.g. @connect). e.g.:

@connect({
  mapStateToProps: (state, props) => ({
    user: readObjectById(state, props.id),
  }),
})

In both cases the store's state is provided to us, and it seems a little risky to assume that apollo client will be in sync at all times with the state being provided.

[Poincare]

I'm not sure what you mean by the last point. Are you saying that there may be parts of Apollo Client or parts of the application that are operating on a previous state of the store? I don't think that could be the case since we are dispatching actions to the store synchronously.

[nevir]

Well, for example, react-apollo's @connect evaluates store state when the store's change handler fires. It then takes all the calculated props and hangs them off itself; but allows React to manage when the decorated component next renders (via this.setState).

That re-render happens asynchronously some time later (possibly much later, for applications that rely on custom React batching strategies such as once-per-frame rendering). After some delay the decorated component would then re-render w/ the updated props - but the store could have changed many times in the meantime.

My worry is that, if I'm naively calling this.context.client.readObjectById(id) during the render() of a component, it is very likely to receive data that is out of sync with the props that were received via @connect.

It would be much preferable, IMO, to fetch the object while @connect evaluates all of its props. And having a helper that can just consume the state object already pulled out for mapStateToProps would be pretty natural, and hopefully guide folk towards good behavior. (Or maybe a @connect specific abstraction)

[Poincare]

Ah, I see what you mean. So I guess it is somehow more convenient to operate on the state object through react-apollo rather than the client instance. I guess state can just be an optional argument to readObjectById and that'll allow for it to be used within the @connect block.

[nevir]

So I guess it is somehow more convenient to operate on the state object through react-apollo rather than the client instance.

Purely because we wouldn't need a reference to the apollo client itself - only this read helper (I think)? If not, then maybe a different pattern is needed

[stubailo]

I feel like this method should take a graphql fragment as an argument, so that you can read nested data originating at this location in the store, and you can get a result in the result format rather than serialized arguments, ID references, etc.

[stubailo]

This key will sometimes contain serialized arguments, we should add some tests about how it will be to read out data that has fields with arguments in it.

[Poincare]

@stubailo After thinking about using fragments vs. this approach, I agree with going w/ the fragment-based approach.

For the sake of recording the decision, I'll describe why the current approach doesn't make sense. Say you fire queries that look like this:

query($friendId: Int) {
   user {
     name
     friend(id: $friendId) {
       name
     }
   }
}

If this query is fired for multiple friendIds and then we do a readObjectById on the id given to a particular user, it is totally non-obvious how we would go about combining the different "friend" values that exist if we want the object that's read to look like a GraphQL result. You can imagine putting them all into a list in this case, but this gets more complicated if there's an argument to friend like type (e.g. CLOSE or DISTANT as enum values). This means that constructing a result object out of these is pretty application-specific.

On the other hand, if we use a fragment, we can avoid all of this pain. For the original example (mentioned in #474 ), we can still do something that looks like:

client.readObjectFromId(id, gql`
fragment {
  name
  friends {
    name
  }
}`);

This will reach into object with the id of id and resolve all of the fields that we want.

I'll fix up this PR accordingly.

[stubailo]

Any reason we need this function anymore? Seems like this is just a clone of readFragmentFromStore?

[Poincare]

@stubailo

I currently use it for all of the unit tests. The main point is to give it a name that reflects the name exposed to the app developer through Apollo Client. Maybe readFragmentFromStore might do something slightly different in the future so having the unit tests call readObjectByIdFromStore instead (which is a name that describes what is happening) may be a good idea.

[nevir]

If I'm reading the patch correctly, it doesn't appear like this PR reads anything beyond the data (NormalizedCache) portion of Apollo's state?

Which, I think, means this method won't reflect any optimistic updates? (it probably should!)

---

We just ran into that same problem when trying to consume values via readQueryFromStore. Currently working around that by reaching into apollo-client/lib/src/store for getDataWithOptimisticResults.

[Poincare]

I assume that optimistic updates probably don't have the same ids as the values returned from the server in most cases - given this, it probably doesn't make sense to roll both of them into the same method.

[nevir]

I assume that optimistic updates probably don't have the same ids as the values returned from the server in most cases - given this, it probably doesn't make sense to roll both of them into the same method.

We often do. For example, updates to existing/known entities (the current user's preferences, optimistically changing the state of some already retrieved resource, etc)

[stubailo]

This could be obviated by #617

[helfer]

I'm going to close this for now, we should take care of it in the refactor.