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

Support for tree shaking in the __Schema.possibleTypes() #5808

Closed
rohandhruva opened this issue Apr 13, 2024 · 12 comments · Fixed by #5823
Closed

Support for tree shaking in the __Schema.possibleTypes() #5808

rohandhruva opened this issue Apr 13, 2024 · 12 comments · Fixed by #5823

Comments

@rohandhruva
Copy link
Contributor

Use case

We have configured apollo-kotlin gradle plugin with alwaysGenerateTypesMatching.set(listOf(".*")). This is so that we can match against union or interface types which don't have explicit spreads but we can use the generated .type.name to match against __typename.

Originally, we also had generateSchema = true, so that we could use __Schema.possibleTypes(), which is very useful for enumerating over interface or union types. However, because possibleTypes() operates at runtime, it's not possible for R8 or proguard to strip unused types from the android app APK.

Would it be possible to add support for being able to tree shake unused types out of the generated __Schema file? Maybe a middle ground is that we specify all the possibleTypes usages in our gradle configuration, and only those are generated, e.g. __Schema.possibleTypesOfFoo().

Describe the solution you'd like

No response

@martinbonnin
Copy link
Contributor

Thanks @rohandhruva !

Would something like below work?

public object __Schema {
  public fun nodePossibleTypes(): List<ObjectType> = listOf(Node, Product, Review)
  public fun productPossibleTypes(): List<ObjectType> = listOf(Product)
  public fun reviewPossibleTypes(): List<ObjectType> = listOf(Product)
  public fun shippingInformationPossibleTypes(): List<ObjectType> = listOf(ShippingInformation)
}

Or are you operating on strings or ObjectType? Would it be easier to have plain strings instead maybe?

public object __Schema {
  public fun nodePossibleTypes(): List<String> = listOf("Node", "Product", "Review")
  public fun productPossibleTypes(): List<String> = listOf("Product")
  public fun reviewPossibleTypes(): List<String> = listOf("Product")
  public fun shippingInformationPossibleTypes(): List<String> = listOf("ShippingInformation")
}

Can you share an example of how you're planning to use it at the callsite?

@rohandhruva
Copy link
Contributor Author

Since we removed the code-gened __Schema, we have just been using List<String> as the return type. However, returning ObjectType seems ok because it is fairly straightforward to use .map { it.name } (which is what we were doing in the past when we used the generated __Schema).

Can you share an example of how you're planning to use it at the callsite?

From a quick look at our code, most of the current usage is for cache normalization in cacheKeyForObject, so we had to match it against a string. In the obj: Map<String, Any?> param, we look up the value for key __typename, and match it to a list of possible types. For example, all types of the Video interface share the same logic for generating the cache key.

@martinbonnin
Copy link
Contributor

we look up the value for key __typename, and match it to a list of possible types

Almost feels like using the opposite of possibleTypes() is better for this use case?

  override fun cacheKeyForObject(obj: Map<String, Any?>, context: CacheKeyGeneratorContext): CacheKey? {
    val keyFields = context.field.type.rawType().keyFields()

    val typename = obj["__typename"]

    if (__Schema.getObject(typename).implements.map { it.name }.contains("Video")) {
      // do sometihng
    }

implements encodes the exact same information as possibleTypes except it's more "direct". An unused object in the type hiearchy will not be pulled in the inheritance graph. Or do you need possibleTypes as well?

@rohandhruva
Copy link
Contributor Author

@martinbonnin thank you so much for that suggestion. To be honest, I had completely missed the context param and our code was just looking at the obj map.

@rohandhruva
Copy link
Contributor Author

rohandhruva commented Apr 15, 2024

I will look into changing the cache key resolver based on context, however I also noticed that we have other usages of possibleVideoTypes. For example, to fetch the videoId from a given Entity in the schema, we do a when on the __typename:

    override fun getEntityId(): String {
        return when (entity.unifiedEntity?.__typename) {
            in possibleVideoTypes -> {
                entity.unifiedEntity?.onVideo?.videoSummary?.videoId?.toString().orEmpty()
            }
            in possibleGameTypes -> {
                entity.unifiedEntity?.onGame?.gameSummary?.gameId?.toString().orEmpty()
            }
            else -> {
                ""
            }
        }
    }

Note that for now possibleVideoTypes and possibleGameTypes are a "static" List<String> that we maintain.

Do you think we could easily use .implements here? Or probably not?

@martinbonnin
Copy link
Contributor

I had completely missed the context param and our code was just looking at the obj map.

I did too and we were right to forget about it. context.field.type is not what you want. This is the type of the field (possibly interface) and not the concrete object type being returned.

{
  video { # type is interface Video
     __typename # typename is object Movie
  }
}

So you'll need something like __Schema.object(typename) in all cases.

Regarding your sample, sounds like something that could be using the super types too instead of the possible types:

    override fun getEntityId(): String {
        val superTypes = __Schema.object(entity.unifiedEntity?.__typename).implements.map { it.name }.toSet()
        return when  {
            superTypes.contains("Video") -> {
                entity.unifiedEntity?.onVideo?.videoSummary?.videoId?.toString().orEmpty()
            }
            superTypes.contains("Game") -> {
                entity.unifiedEntity?.onGame?.gameSummary?.gameId?.toString().orEmpty()
            }
            else -> {
                ""
            }
        }
    }

Note that the two branches above are theorically not disjoint. Something could be both a Video and a Game if both of those are interfaces

@rohandhruva
Copy link
Contributor Author

While all of this makes sense, I think as soon as we have __Schema.object(), with a string based lookup, theoretically we can't tree-shake any of the unused types, can we?

@martinbonnin
Copy link
Contributor

While all of this makes sense, I think as soon as we have __Schema.object(), with a string based lookup, theoretically we can't tree-shake any of the unused types, can we?

Right 👍 Unless R8 is smart enough for that but I have doubts...

Oh my... I'm starting to doubt everything now. Can we even treeshake the possibleTypes? If we are using ObjectType:

public fun videoPossibleTypes(): List<ObjectType> = listOf(Movie, /* all (possibly) unused types here */)

In the Video case all the subtypes of Video need be kept, even if unused?

At least Strings mitigate this a litlle bit

public fun videoPossibleTypes(): List<String> = listOf("Movie", /* all (possibly) unused types here but only as Strings*/)

@rohandhruva
Copy link
Contributor Author

rohandhruva commented Apr 17, 2024

In the Video case all the subtypes of Video need be kept, even if unused?

I think that sounds reasonable, assuming you are using possibleVideoTypes, you are expecting all subtypes of Video to possibly show up in the response.

I think we might still be able to have R8 remove all the types that are completely unused from the schema and possibleTypes, right?

@martinbonnin
Copy link
Contributor

assuming you are using possibleVideoTypes, you are expecting all subtypes of Video to possibly show up in the response.

👍

I think we might still be able to have R8 remove all the types that are completely unused from the schema and possibleTypes, right?

Sounds like it, I'll dig!

@martinbonnin
Copy link
Contributor

Pull request here

Copy link
Contributor

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Kotlin usage and allow us to serve you better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants