sysgears / apollo-universal-starter-kit

Apollo Universal Starter Kit is a SEO-friendly, fully-configured, modular starter application that helps developers to streamline web, server, and mobile development with cutting-edge technologies and ultimate code reuse.
https://apollokit.org
MIT License
1.68k stars 323 forks source link

Update instead of updateQueries #140

Closed larixer closed 7 years ago

larixer commented 7 years ago

Based on discussion with Apollo devs the usage of react-apollo updateQueries is considered to be deprecated in favor of http://dev.apollodata.com/core/read-and-write.html

This is not deprecated explicitly yet, but the general agenda is to use Direct Cache Access methods instead of old mechanisms.

We should update this starter kit this new approach.

mitjade commented 7 years ago

@vlasenko I agree, but at the time I was writing post example, I was not able to make it work. I think it was a __typename problem with apollo that was since fixed.

I actually like the reducer approach better, since you can update the store in one place for mutations and subscriptions. Now you have to handle this in multiple locations. But again was not able to make it work.

Would love to give this another try, if someone would help me in case I again get stuck.

So what approach would you prefer and why?

larixer commented 7 years ago

@mitjade I think Direct Cache Access is the newest and preferred method, we should use it, hopefully it can solve all our cache manipulation goals. And if not, we need to think and possible reach out to Apollo devs.

mitjade commented 7 years ago

@vlasenko OK. Will try to prepare a PR for this.

larixer commented 7 years ago

@mitjade

I actually like the reducer approach better, since you can update the store in one place for mutations and subscriptions. Now you have to handle this in multiple locations. But again was not able to make it work.

Could you point me into the relevant section of Apollo docs what are you referring to ?

mitjade commented 7 years ago

http://dev.apollodata.com/react/cache-updates.html#resultReducers

and the article of Sacha Greif

https://blog.vulcanjs.org/understanding-post-mutation-data-updates-in-apollo-43e90c37023b

larixer commented 7 years ago

@mitjade Yeah, reducer approach is interesting, especially when we have both subscriptions and mutations

mitjade commented 7 years ago

@vlasenko I would actually like to try both options and see what would in the end be the best solution. I can prepare PR of what I am able to come up with and then with some of your help we can come up with a solution that would make the most sense.

larixer commented 7 years ago

@mitjade Yes, sounds great! It is not clear now what is best to use from various approaches to manipulate query cache. Seems that Direct Cache Access is preferred, but there is no replacement for reducer functionality there, I will try to ask on Apollo slack channel about reducer way, whether it is still recommended to use it or not

mitjade commented 7 years ago

@vlasenko At the bottom of https://dev-blog.apollodata.com/apollo-clients-new-imperative-store-api-6cb69318a1e3 Caleb Meredith mentions the following:

Personally, I don’t think reducers are a good option because they depend on Apollo Client’s Redux internals which may not be around forever, but I know some people see merit in reducers and that’s fine.

mitjade commented 7 years ago

@vlasenko I actually think we could combine reducer with readQuery() and writeQuery(), to get the best of both worlds. But this I just an idea I have. So lets try and see what can be done.

larixer commented 7 years ago
helfer [8:43 PM] 
vlasenko Personally I would just use `update` for everything. I’m not sure about this yet, but I think we may deprecate the other functions in the future (too many options is confusing, and it’s harder to maintain).

vlasenko [8:45 PM] 
helfer Yeah, looks like there are too much of options available. So it really makes sense! Thanks a lot!
larixer commented 7 years ago

@mitjade I think we should use only Direct Cache Access methods, because Jonas Helfer told they are going to deprecate all the other ways to manipulate query cache.

mitjade commented 7 years ago

@vlasenko Looks like it. I just hope that they then also provide a better solution on how to update mutations and subscriptions together.

larixer commented 7 years ago

@mitjade I hope so too. Hopefully they provide a proper reducer option too.

mitjade commented 7 years ago

@vlasenko I gave it a quick try and was able to make it work for comment add mutation. Will try to finish the rest tomorrow and make a PR.

larixer commented 7 years ago

@mitjade Sounds great! Thanks!

mitjade commented 7 years ago

I have a problem with update. How would one use it with subscriptions? http://dev.apollodata.com/react/api-queries.html#graphql-query-data-subscribeToMore

Docs say: In order to update the query’s store with the result of the subscription, you must specify either the updateQuery option in subscribeToMore or the reducer option in your graphql() function.

larixer commented 7 years ago

@mitjade Yeah, looks like there is no way to do it at the moment: https://github.com/apollographql/apollo-client/blob/master/src/core/watchQueryOptions.ts#L84-L92

larixer commented 7 years ago

In the same file: https://github.com/apollographql/apollo-client/blob/master/src/core/watchQueryOptions.ts#L156

Looks like they have added Direct Cache Access interface to mutations and not to subscriptions :)

mitjade commented 7 years ago

@vlasenko So do we leave updateQueries for now and ask how this could be handled with update. I would also like to hear how they see handling mutations and subscriptions together?

larixer commented 7 years ago

@mitjade Mitja could you investigate using reducer option, will it result in more compact code for our subscriptions and mutations?

mitjade commented 7 years ago

@vlasenko I think it would, but would need to test it out. Will try over the weekend.

I am also having trouble how to correctly unsubscribe from subscribeToMore https://github.com/apollostack/apollo-client/blob/master/src/core/ObservableQuery.ts#L269-L275

larixer commented 7 years ago

@mitjade I'm not quite understand you. I'm working on tests for React components, including GraphQL logic, so perhaps I will face this problem too.

mitjade commented 7 years ago

@vlasenko In post example I wanted to use:

  componentWillUnmount() {
    if (this.subscription) {
      this.subscription.unsubscribe();
    }
  }

Like you did in counter example, but I get an error this.subscription.unsubscribe is not a function.

Side-effect is that you remain subscribed to the last edited post. It is not a big problem, but would like to do it right. They mention unsubscribe in http://dev.apollodata.com/react/api-queries.html#graphql-query-data-subscribeToMore, but it does not work for me?

larixer commented 7 years ago

@mitjade

I am also having trouble how to correctly unsubscribe from subscribeToMore https://github.com/apollostack/apollo-client/blob/master/src/core/ObservableQuery.ts#L269-L275

Mitja, I think you are referring to wrong lines in ObservableQuery. This comment was made by Tom on Jan 19th.

Here are the lines Tom referred to: https://github.com/apollographql/apollo-client/blob/f3882b976a9146fb4826efc2951dc16cd90326c8/src/core/ObservableQuery.ts#L269-L275

These lines are available in the master now too, here: https://github.com/apollographql/apollo-client/blob/65595622284ffc9d08d39f083438063ab14f83a6/src/core/ObservableQuery.ts#L294-L300

So if I guess what Tom meant by referring to these lines is to use code like this to unsubscribe:

const unsubscribe = subscribeToMore(...);
unsubscribe();
mitjade commented 7 years ago

@vlasenko So what I need to do is:

  componentWillUnmount() {
    if (this.subscription) {
      this.subscription();
    }
  }
larixer commented 7 years ago

@mitjade Yep, looks like this

mitjade commented 7 years ago

@vlasenko Initial proposal for reducer implementation #141.

larixer commented 7 years ago

@mitjade Mitja, you did great work to investigate reducer option, I appreciate it very much!

I would say reducer option is even less clean then updateQueries, or update, looks very low-level. Among different options I like update much more then any other option, because you don't stick to the name of the query, with update you are using query itself to manipulate cache which will reduce confusion in big projects . Now I understand why Apollo devs want to go with update route, but they don't implement it for subscription yet. Maybe we can help them? :) At the moment I would vote for using update where we can and updateQueries in other places.

mitjade commented 7 years ago

@vlasenko OK, but until we have an option to do mutations and subscriptions with update, I would not change the current code, since you would then need to implement 2 different ways to update client cache.

larixer commented 7 years ago

@mitjade Yes, I fully agree with you!

larixer commented 7 years ago

Closing this issue until Direct Cache Access API will be available for subscriptions too.

mitjade commented 7 years ago

Doing the reducer and pagination example, I now slowly understand why relay forces you do have well defined queries, inputs and payloads. So you then have less work down the line. But at least with Apollo you can create your own conventions.