Migration 1.0 - Questions & Feedback

[wow-such-amazing]

Question

Hey folks!

I've started to migrate the project to Apollo version 1.0.5 from 0.53.0. I would like to be able to make the project running as soon as possible and so I have a question regarding the migration.

Namespace

Is it possible to disable types namespace? As we have many type extensions, queries and mutations it's quite a lot of work to add an extension for each type that is used in the project. Also I'm not sure if I can see exactly the benefit for our project right now. So is it possible to not use a namespace?

Types + CamelCase

Another question is whether it's possible to provide an option to use camel case for types? Similar to what there is for enums. So that we have less errors to fix and as well to have a camel case style 🤔

Old way:

context.data.coreNotificationCounter

New way:

context.data.core_notificationCounter

[wow-such-amazing]

Fragments + Properties

Another question/feedback 🌚

We have a fragment that is defined like this:

fragment ProductBasicFragment on Core_Product {
  ...ProductBasicSharedLevelFragment
  ...ProductBasicCommunityLevelFragment
  withEvent(eventId: $eventId) @skip(if: $eventIdIsNil) {
    ...ProductBasicEventLevelFragment
  }
}

We also have some functions that convert that fragment into domain model. For example one of the functions is:

func name() -> String {}

Previously this was working well. But now it throws an error about invalid redeclaration because name exists on the Core_Product object. And seems like it happens because ProductBasicFragment provides values from within its fragments. Is it possible to disable that logic? So that name could be accessible only through fragments where it's defined. And not on the top level of ProductBasicFragment?

[wow-such-amazing]

Fragments + Access

I also would like to share my feedback about the way how we should access fragments with a new version. Because right now the refactoring makes such lines much longer than before because we need to add a schema type before accessing the fragment. But the fragment itself is based on some specific type 🤔

For example such code becomes extremely long and I'm not sure how to make it more compact 🤔

if let redirection = fragment.asCore_EventPeopleListView?.fragments.peopleListEventViewFragment.redirection() {
  return redirection
}

And it's actually even longer because the fragment variable is defined like this:

guard let fragment = view.asCore_EventViewInterface?.fragments.eventViewFragment else {
  return nil
}

All of that is based on such graphql definition:

fragment EventDetailsFragment on Core_Event {
  contents {
    views {
      ...EventViewFragment
    }
  }
}

fragment EventViewFragment on Core_EventViewInterface {
  ...EventViewCommonFragment
  ...PeopleListEventViewFragment
}

fragment EventViewCommonFragment on Core_EventViewInterface {
  id
  name
  color
  iconUrl
  backgroundImageUrl
}

fragment PeopleListEventViewFragment on Core_EventPeopleListView {
  id
}

I know PeopleListEventViewFragment is sort of redundant, but that's mostly an example. There are other fragments that contain their type specific data. So what's the reason behind that change?

Here is what we've had before:

let fragments = view.fragments.eventViewFragment.fragments

if let redirection = fragments.peopleListEventViewFragment?.redirection() {
  return redirection
}

This was much easier to read and write imo 🤔

[AnthonyMDev]

Namespace
Is it possible to disable types namespace? As we have many type extensions, queries and mutations it's quite a lot of work to add an extension for each type that is used in the project. Also I'm not sure if I can see exactly the benefit for our project right now. So is it possible to not use a namespace?

The namespace is used in order to prevent naming conflicts. If you don't want to have to use the qualified names of your generated types though, the namespace is not generated if you set your codegen configuration to include your generated types as a separate module. If you use your generated models as a separate module, then the namespace is the module name instead of being an enum inside of your target.

You'll need to import that module in your files that use your models, but not every model will need to be namespaced inline!

Our Project Configuration guide can help make all of this clearer.
If you really want to just remove the namespace, setting your output.schemaTypes.moduleType to .other might do what you want, but we can't guarantee that you won't get compilation errors due to naming conflict if you include the generated types in your application target directly that way.

[AnthonyMDev]

Another question is whether it's possible to provide an option to use camel case for types? Similar to what there is for enums. So that we have less errors to fix and as well to have a camel case style.

I'm not sure exactly what you are talking about here. You say types, but your example looks like it's referring to field names?

[AnthonyMDev]

We also have some functions that convert that fragment into domain model. For example one of the functions is:

func name() -> String {}

Previously this was working well. But now it throws an error about invalid redeclaration because name exists on the Core_Product object. And seems like it happens because ProductBasicFragment provides values from within its fragments. Is it possible to disable that logic? So that name could be accessible only through fragments where it's defined. And not on the top level of ProductBasicFragment?

We are planning on allowing you to turn off merging of fragment fields in an upcoming release. But (without complete context of your project) I'm not sure that this is the best approach.

It seems like your name() function may be trying to get the name field from one of the inner fragments. The whole point of fragment merging is that you shouldn't need to do that, as the name field now exists right on your Core_Product fragment object.

If you want to convert the fragment model into a different model, and you are actually doing some custom transformations of the data, you should probably wrap the entire struct in a new struct, rather than using extensions on our models to convert individual fields. Our generated models are meant to reflect the data as it is fetched and stored in the cache.

[AnthonyMDev]

Fragments + Access
I also would like to share my feedback about the way how we should access fragments with a new version. Because right now the refactoring makes such lines much longer than before because we need to add a schema type before accessing the fragment. But the fragment itself is based on some specific type 🤔

For example such code becomes extremely long and I'm not sure how to make it more compact 🤔

This example actually is part of why fragment field merging is so great and I don't recommend you should turn it off!

The asCore_EventViewInterface? and asCore_EventPeopleListView? are necessary to ensure that the fragments exist. The primary difference here in 1.0 is that instead of making peopleListEventViewFragment? an optional field, we made it so you need to check the type condition first (ie. is at a type that conforms to Core_EventViewInterface) before accessing the fragment, moving the optionality to the type condition, rather than the fragment fulfillment.

We believe that, when you do get nil, this makes it more clear that the reason you got nil is because the object does not fulfill the type condition, rather than getting an empty fragment an not being 100% sure why.

But what fragment merging does should make your code even more compact than before! The View should merge the fragment type condition for the PeopleListEventViewFragment (ie. Core_EventPeopleListView) into the View.AsCore_EventViewInterface) object. And it merges the field from the conditional fragment right up into the object.

So you should be able to write this:

If redirection is a generated field:

let redirection = view.asCore_EventViewInterface?.asCore_EventPeopleListView?.redirection

or to get the PeopleListEventViewFragment:

let peopleFragment = view.asCore_EventViewInterface?.asCore_EventPeopleListView?.fragments.peopleListEventViewFragment

[AnthonyMDev]

Thanks for the feedback! I hope this answered many of your questions. Since there is no current actionable issue here, I'm going to close this issue. But if you have more questions, I am happy to answer them for you on the thread still!

[wow-such-amazing]

Hey @AnthonyMDev! Thanks for the answers! Really appreciate 🙌 I will get back to some of them later, but for now I have a question about the cache migration. If you could shed some light that would be great 🌚

So basically I'm wondering how the cache migration will happen once users install the app with a new version of apollo. Do you drop existing cache or do you migrate it somehow?

I'm asking because this was our old way to provide a cache key:

store.cacheKeyForObject = {
    guard let typename = $0["__typename"] as? String else {
        return nil
    }
    
    if let id = $0["id"] as? String {
        return typename + ":" + id
    } else if let id = $0["_id"] as? String {
        return typename + ":" + id
    } else {
        return nil
    }
}

As you can see as a key we were using a combination of typename + id with a custom : separator. New docs suggest that we don't need to provide a typename manually, but it will be used by default. So we should provide only id. Hence I'm thinking about this implementation:

let id: String? = object["id"] as? String ?? object["_id"] as? String

guard let id = id else {
    return nil
}

return CacheKeyInfo(id: id)

But I'm wondering what will be an effect for the app in that case? And what is the best scenario to minimise any negative effects?

Thanks!

[wow-such-amazing]

I have been testing the migration to 1.0.0 and found that when I update the app to the new version I can see some pages from the cache from the old version while I can't see some others. Not sure yet what could be the problem. I have verified that the fragment is the same in between versions, so if you have ideas what to check that would be great. Maybe I can get some logs in order to get more info on why cached object is not being displayed.

P.S. If I install back again the version from the store then missing pages are displayed from cache again 🤔

[wow-such-amazing]

@AnthonyMDev Coming back to some previous topics 🌚

Namespaces

I've been able to get through the migration, so at this point I think it doesn't bother me that much anymore, so I think we can skip it 😁

Fragments + Properties

It seems like your name() function may be trying to get the name field from one of the inner fragments. The whole point of fragment merging is that you shouldn't need to do that, as the name field now exists right on your Core_Product fragment object.

Not sure how this would work in case if Core_Product has 2 fragments and both those fragments contain an id field. It may throw an error at the code generation step probably 🤔

But in any case it feels more natural to access a property of a fragment through the fragment rather than through the parent of that fragment 🤔

If you want to convert the fragment model into a different model, and you are actually doing some custom transformations of the data, you should probably wrap the entire struct in a new struct, rather than using extensions on our models to convert individual fields. Our generated models are meant to reflect the data as it is fetched and stored in the cache.

That's what we do. We have a dedicated struct that is called ProductModel. What we have is an extension on a fragment type to return that model. So that it's easy to convert fragments into models at the network layer:

extension ProductFragment {
  func model() -> ProductModel {
    return ProductModel(...)
  }
}

The problem is that our current logic based on fragments. And migration requires updating function names in order to prevent collisions because of the fields being merged into a parent. if I understand correctly the reason why it happens 😄

Fragments + Access

We believe that, when you do get nil, this makes it more clear that the reason you got nil is because the object does not fulfill the type condition, rather than getting an empty fragment an not being 100% sure why.

I agree that it could provide more context. Maybe it could be possible to have an option for the codegen to decide which code generation is preferred. Either specifying a type before accessing fragments or accessing fragments directly and having those fragments optional as it was before 🤔

But what fragment merging does should make your code even more compact than before!

I'm not sure 😅 Our code relies on fragment types. We write extensions for fragments in order to return their domain models. So switching everything to be based on the merged fields of the parent types doesn't sound like a solution for a migration, because that would mean for us to re-write all of our existing fragments extensions into extensions on their parent types. I can't imagine how much time this might take 😅 So right now the viable option for us is to add a type from the schema before accessing fragments, which makes the code longer and less clean.

On top of that what I don't like with such approach is that before we were able to hide schema types by defining our fragments. Let's say there is some legacy type on the schema and its name doesn't make sense anymore. Previously we could create a fragment with a better name and then in our swift code use only that fragment. So it looks nicer. But now we would need to specify a legacy type name in any case 🤔

[AnthonyMDev]

As far as the caching stuff goes, our cache is meant to be just that -- a cache. It should be relatively short-lived and not considered a source of truth for data. The functionality is not robust enough to be a complete persistence solution right now. (I hope that at some point in the future, we can make it that powerful though!)

The cache should be hydrated with data from your server and re-hydrated on a regular basis. The mechanisms for flushing stale cache data are also not full fleshed out right now, so clearing your cache and re-hydrating on a semi-regular basis might not be a bad idea.

All of this is to say, we do not have an official mechanism in place for migrating the cache. If at all possible, I would recommend clearing the cache and starting over by re-fetching all data you need after migrating to Apollo 1.0.

The normalized cache implementation has not changed significantly, so theoretically, old cache data should still work with the new version for the most part. Provided your cache keys are configured the same. In the examples you provided, it looked this the cache keys should still be the same after the migration!

Glad to hear that it's at least working for some of your data. For the data that isn't getting cache hits, some significant debugging might be necessary if you really want to save that data. I'm not sure what exactly could be causing that to happen. You'll want to try opening up your .sqlite db file in a db viewer and inspect the cache keys for the objects. Set some breakpoints in the GraphQLExecutor and see if you can figure out what cache keys the executor is expecting and where they mismatch.

But we aren't going to consider this as a "bug" that we plan on fixing in the near future, as we don't intend for the cache to be a complete long-term offline persistence mechanism currently.

[AnthonyMDev]

Not sure how this would work in case if Core_Product has 2 fragments and both those fragments contain an id field. It may throw an error at the code generation step probably 🤔

But in any case it feels more natural to access a property of a fragment through the fragment rather than through the parent of that fragment 🤔

We deduplicate fields, so this wouldn't cause any errors! You can still access the fragment as you wish, and if you really just want to use the fragments, we will be adding the ability to turn off the fragment field merging. It's on our roadmap of currently planned work.

The problem is that our current logic based on fragments. And migration requires updating function names in order to prevent collisions because of the fields being merged into a parent. if I understand correctly the reason why it happens 😄

This is only a problem if you are naming your functions the exact names of individual fields. I think a func model() -> ProductModel makes sense, but in your first example, you had a function called name(), which looks like you are trying to create these extensions for individual fields. I'd recommend only having these functions for the overall model, rather than for each field.

This is personal design preference, but I also would use names that indicate that a conversion is happening more. So I'd call it toModel() or even just have an initializer for the model ProductModel(_ fragment: ProductFragment) so that your code is more clear in what it's doing when you do these conversions. But again, that's just me!

Also, you could just as easily call model() on the distinct model instead of the fragment if you wanted to. Using protocol extensions you could do something like this:

protocol ProductModelConvertible {
  var productFragment: ProductFragment { get }
}

extension ProductModelConvertible {
  func toProductModel() -> ProductModel {
    let fragment = productFragment
    return ProductModel(...)
}

extension ProductFragment: ProductModelConvertible {
    var productFragment: ProductFragment { self }
}

extension MyQuery.Data.Item.Product: ProductModelConvertible {
    var productFragment: ProductFragment { self.fragments.productFragment }
}

This can be made to use optionals to handle type conversion checks, or expanded and composed in any way that works for your project as well.

I agree that it could provide more context. Maybe it could be possible to have an option for the codegen to decide which code generation is preferred. Either specifying a type before accessing fragments or accessing fragments directly and having those fragments optional as it was before 🤔

This is not something we are going to consider adding without a significant amount of community feedback asking for it. What we have today does not block anyone from doing what they want, provides all the benefits that I outlined above, and maintains consistency. Trying to handle all of the edge cases when having to handle type conditions in two different ways would make maintaining the code generation engine and the generated model shape really difficult and could limit us in future feature development too much.

I'm not sure 😅 Our code relies on fragment types. We write extensions for fragments in order to return their domain models. So switching everything to be based on the merged fields of the parent types doesn't sound like a solution for a migration, because that would mean for us to re-write all of our existing fragments extensions into extensions on their parent types. I can't imagine how much time this might take 😅 So right now the viable option for us is to add a type from the schema before accessing fragments, which makes the code longer and less clean.

Totally makes sense for an existing codebase! And like I said, you are still able to do so! We didn't remove any capabilities here, and at some point in the near future we'll allow you to turn off the fragment field merging.

On top of that what I don't like with such approach is that before we were able to hide schema types by defining our fragments. Let's say there is some legacy type on the schema and its name doesn't make sense anymore. Previously we could create a fragment with a better name and then in our swift code use only that fragment. So it looks nicer. But now we would need to specify a legacy type name in any case 🤔

---

It sounds like your biggest concerns were the conflicting of field names with your conversion functions (which I think is best solved by restructuring your conversion functions) and having to call through the type conditions to get to your fragments (which is a bit more code, but doesn't prevent you from doing what you want).

1.0 was supposed to be a major breaking change. We fundamentally re-thought a lot of old assumptions to provide an API that we could build upon in a safe way without having to make large breaking changes in the future. That means that the 1.0 migration is going to be a bit more painful for people. This was intentional -- we wanted to rip the band-aid off all at once.

I think that if you had the API of 1.0 when creating your fragment-to-model conversion code in the first place, you might have structured it differently, and in my opinion, possibly in a cleaner way. I know at my previous job, where I was using Apollo iOS 0.47.0, we had a ton of messy, complex, semi-hacky code to convert the generated models into view models. The old APIs lent themselves to (and oftentimes forced you to) writing some nasty code.

Trying to refactor your existing infrastructure to use 1.0 is probably going to lead to some awkward cases. But the goal was that if you are creating a new project from scratch (or if you invest the time in really re-thinking some of your app code instead of trying to do the minimal necessary to migrate to 1.0), that you would come out with higher quality code.

Making the 1.0 migration as painless and easy as possible was not our intention. The goal was to get to a stable API that encourages our users to write better code in the first place. I don't think we are 100% there yet, there are still rough edges around local cache mutations and complex networking configuration (among other areas).