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

Add refetchQueries to mutations #884

Open
andr-ec opened this issue Nov 4, 2019 · 9 comments
Open

Add refetchQueries to mutations #884

andr-ec opened this issue Nov 4, 2019 · 9 comments
Labels
caching enhancement Issues outlining new things we want to do or things that will make our lives as devs easier networking-stack

Comments

@andr-ec
Copy link

andr-ec commented Nov 4, 2019

The apollo Android API has a modifier to a mutation called refetchQueries that takes a list of queries as arguments and refetches those queries after the mutations is completed. I would love to see this added to .perform as an optional argument.

Right now the alternatives are:

  • Manually modify the cache based off the response
  • Clear the whole cache
  • Manually refetch the queries using .fetchIgnoringCacheData
  • Change the response on the server side to include the updated queries.

All of these work. We personally refetch the queries on our iOS project. But I feel like specifying which queries to refetch would lower the complexity of mutations. And we really liked using refetchQueries on the android side.

@designatednerd designatednerd added enhancement Issues outlining new things we want to do or things that will make our lives as devs easier caching labels Nov 4, 2019
@designatednerd
Copy link
Contributor

Interesting - this is one of the side effects of using the query path as the cache key by default. I'm tied up a bit at the moment in terms of adding it myself but I'd be more than happy to look at a PR for this!

If you don't have bandwidth then note to self that this would probably be a good one to implement in concert with #192, query batching, since then we could send all the queries that need to be updated in one massive pile.

@andr-ec
Copy link
Author

andr-ec commented Nov 8, 2019

I would love to implement this, I'll take a look today!

@andr-ec
Copy link
Author

andr-ec commented Nov 8, 2019

I started on the PR and I ran into a couple of implementation questions. Where is the best place to add the logic for the refetch queries? I initially came up with this but I'm thinking I will need need some deeper changes.

  @discardableResult
  public func perform<Mutation: GraphQLMutation, Query: GraphQLQuery>(mutation: Mutation,
                                                                      refetchQueries: Query...,
                                                 context: UnsafeMutableRawPointer? = nil,
                                                 queue: DispatchQueue = DispatchQueue.main,
                                                 resultHandler: GraphQLResultHandler<Mutation.Data>? = nil) -> Cancellable {
    
    return self.send(operation: mutation,
                     shouldPublishResultToStore: true,
                     context: context,
                     resultHandler: wrapResultHandlerForQueries(wrapResultHandler(resultHandler, queue: queue), queries: refetchQueries, context: context))
  }
  
  private func wrapResultHandlerForQueries<Data, Query>(_ resultHandler: GraphQLResultHandler<Data>?, queries: [Query], context: UnsafeMutableRawPointer? = nil) -> GraphQLResultHandler<Data> where Query: GraphQLQuery {
    guard let resultHandler = resultHandler else {
      return { _ in }
    }
    
    return { result in
      if case .success(_) = result {
        _ = queries.map{ operation in
          self.send(operation: operation, shouldPublishResultToStore: true, context: context) { _ in}
        }
      }
        resultHandler(result)
    }
  }

One issue is that cancellables are being created for the queries but can't be used. So does that mean I should be thinking about a deep change to the requests? and the other problem is that queries are expected to be of one type. So I can't create an array of queries like this:

let query1 = HeroAndFriendsNamesQuery()
  let query2 = HeroAndFriendsNamesWithIDsQuery()
  let queries: [GraphQLQuery] = [query1, query2]

@designatednerd
Copy link
Contributor

Right now for more advanced things like refetches we're just sort of dropping those cancellables on the floor (for example, when retrying in response to a retry delegate), so I don't think not returning those cancellables is the end of the world.

One thing to watch out for is assuming that this feature will be used by everyone. Because of this, I think Query... might not be too great for the main API since it doesn't allow you to default to nil the way [Query]? does. There's also the fact that I don't think we're doing a lot of Query... style APIs at the moment.

I think you're generally in the correct spot, but I think making sure that pieces that people may not want to use are optional is key here.

@andr-ec
Copy link
Author

andr-ec commented Nov 11, 2019

Cool I can made the function:

  public func perform<Mutation: GraphQLMutation, Query: GraphQLQuery>(mutation: Mutation,
                                                                      refetchQueries: [Query]? = nil,
                                                 context: UnsafeMutableRawPointer? = nil,
                                                 queue: DispatchQueue = DispatchQueue.main,
                                                 resultHandler: GraphQLResultHandler<Mutation.Data>? = nil) -> Cancellable {

as for creating queries let queries: [GraphQLQuery] = [query1, query2]
won't work because it gives:
Protocol 'GraphQLQuery' can only be used as a generic constraint because it has Self or associated type requirements

So my assumption is that the only solution is type erasure? Am I correct on that assumption?

@designatednerd
Copy link
Contributor

I think if you just pass [query1, query2] in without declaring a separate let, that should work since the Query in the above method defines the generic constraint.

@andr-ec
Copy link
Author

andr-ec commented Nov 11, 2019

Looks like that doesn't work either.
it gives the error: Generic parameter 'Query' could not be inferred

But it does work if there is only one query: [query1]

looks like the issues come from associatedtype Data: GraphQLSelectionSet within the protocol GraphQLOperation.

@designatednerd
Copy link
Contributor

@acarrera94 Please check out the RFC for the networking rewrite here: #1340 - that gives considerably more flexibility to what you can do with the networking stack, and would be curious to hear if you feel it can address the use case here sufficiently or not.

@andr-ec
Copy link
Author

andr-ec commented Aug 22, 2020

Yes that should do it. The biggest issue was the the handling of associated types and generics. Looks like you're addressing that. I'm exited to contribute when it's closer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
caching enhancement Issues outlining new things we want to do or things that will make our lives as devs easier networking-stack
Projects
None yet
Development

No branches or pull requests

3 participants