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

Codegen config for enabling full mutability of response models #3246

Open
KoCMoHaBTa opened this issue Oct 2, 2023 · 3 comments
Open

Codegen config for enabling full mutability of response models #3246

KoCMoHaBTa opened this issue Oct 2, 2023 · 3 comments
Labels
codegen Issues related to or arising from code generation feature New addition or enhancement to existing solutions medium-priority

Comments

@KoCMoHaBTa
Copy link

Use case

Hi,

We are trying to migrate from Apollo 0.53 to 1.5
So far, the main struggle we have is with the cache mutability.
We heavily relay on all types being mutable (which is no longer the case since v1) for

  1. our pagination logic for connections
  2. updating single or few fields on a given type

While small cache mutations for a single type can easily be migrated to using mutable fragments of the given type, this does not work for entire Queries containing a connection where we need to mutate (add/remove) the nodes. We have an abstraction that can handle connection pagination in a generic way with a single query watcher, but it relays on the ability to mutate the types.

TL;DR;

The problem - we wan't to be able to use same types for performing queries and mutating their data into the cache.
The request - add the ability, via codegen config or some custom directive or some other way, to enable full mutability to any type within any query.

Here is an example (simplified, non-generic):

  • consider the following query & fragments:
query ItemsPage(
  $accountId: ID!,
  $after: String
) {
  account(id: $accountId) {
    id
    items(first: 20, after: $after) { ...ItemsPage }
  }
}

fragment ItemsPage on ItemConnection  {
  pageInfo { hasNextPage endCursor }
  nodes { ...ItemFragment }
}

fragment ItemFragment on Item {
  id
  # ect...
}
  • we have a screen that shows the the item connection in a list
  • the item list initially loads the first page of the connection by watching the initial query - this ensures that any mutation updating individual items will get reflected into the list
client.watch(query: ItemsPageQuery(accountId: "1", after: nil)) { result in
  self.items = try result.get().data?.account.items.fragments.itemsPage.nodes.map(\.fragments.itemFragment)
}
  • when scrolling to the bottom we fetch the next page and append the result to the cache of the first page - this allow us to have single query watcher for the whole connection
client.fetch(query: ItemsPageQuery(accountId: "1", after: "p1")) { result in
  client.store.withinReadWriteTransaction { transaction in
    try transaction.update(query: ItemsPageQuery(accountId: "1", after: nil)) { data in
      data.account.items.fragments.itemsPage.nodes.append(contentsOf: try result.get().data?.account.items.fragments.itemsPage.nodes ?? [])
    }
  }
}
  • the same happens for every new page loaded - we insert the result into the cache of the first watched query
  • the list can navigate into item details screen, which can perform mutations on a selected item from any page, which gets automatically reflected into the list thanks to the query watcher.

What we have tried so far

  1. Using local cache mutations & mutable fragments - because they can't be used as queries (can't be fetched) - we have to duplicate every single query that we use to fetch a connection - we have about 40-50 of these - so it's a lot of graphql duplication, which e have to keep in sync in case of anything changes - not a good option. In addition to that - we hit that weird precondition , which says mutate properties of the fragment instead.
  2. Using only mutable fragments - this turned out to be a dead end, because the query data is still immutable, so fragments can't be mutated as part of the query and there is no way to read/write the ItemsPage fragment, because it has no id -> there is no CacheKey for it.
  3. Using @dynamicMemberLookup wrapper that adds setters - this kind of works, but it's very error prone at runtime + it messes up with the cache keys for nested types, so it looks unreliable.
  4. Forking the code and overriding the isMutable in the codegen to return true - this appears to works, but it would be great if it was possible to configure that from the codegen config or custom directive (not producing local cache mutation). We would like to avoid using a fork for the long term, because we will have to constantly sync it with the main repo changes.

Describe the solution you'd like

Add the ability, via codegen config or some custom directive or some other way, to enable full mutability to any type within any query.

@KoCMoHaBTa KoCMoHaBTa added the feature New addition or enhancement to existing solutions label Oct 2, 2023
@AnthonyMDev
Copy link
Contributor

Thank you for the suggestion and detailed use case! I can see there is a lot of support for a feature like this. I know that the move to LocalCacheMutations has been a pain point for a number of users. It's becoming more and more obvious that the flexibility that local model mutability provides is something people really want.

There are some significant trade-offs to this and some added complexity of the generated models. I'm going to take some time to think through the right approach to this. Thanks again for the well written issue!

@AnthonyMDev AnthonyMDev added this to the Minor Releases (1.x) milestone Nov 10, 2023
@AnthonyMDev
Copy link
Contributor

We've discussed this and believe that the way forward is to:

  • Deprecate @apollo_client_ios_localCacheMutation and the associated types in ApolloAPI
  • Add a new @mutable (naming TBA?) directive that can be added to operation and fragments definitions

Allocating time to complete this work is TBD.

@AnthonyMDev AnthonyMDev added codegen Issues related to or arising from code generation medium-priority labels Nov 10, 2023
@AnthonyMDev AnthonyMDev changed the title Codegen config for enabling full mutability on types Codegen config for enabling full mutability of response models Jan 5, 2024
@nuke-dash
Copy link

I would just like to add my experience to this topic.
We migrated from pre 1.0 in which all properties were mutable.
We adjust some properties in our production code base, so not having the properties being mutable would not have been that big of an issue.
But we also have thousands of unit tests which rely on being able to adjust properties without having to initialize an entire object with new properties.
And we wrote our own code generator using sourcery to write extensions with empty inits.
So we can create a GraphQL object without parameters and then adjust just the properties we need for a given test.
Ultimately I forked the codegen and overrode isMutable. (thanks @KoCMoHaBTa for the inspiration 💯 )
This seems to work and causes the least friction transitioning to the latest version.

Incase anyone is interested: zappwaredotcom/apollo-ios-codegen@f918c47

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codegen Issues related to or arising from code generation feature New addition or enhancement to existing solutions medium-priority
Projects
None yet
Development

No branches or pull requests

3 participants