-
Notifications
You must be signed in to change notification settings - Fork 656
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
Improve relocation in gradle plugin #4528
Conversation
✅ Deploy Preview for apollo-android-docs canceled.
|
} | ||
|
||
public final class com/apollographql/apollo3/ast/introspection/IntrospectionSchema$Companion { | ||
public final fun serializer ()Lkotlinx/serialization/KSerializer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an option to hide this from the public API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked a question here
...pollo-ast/src/main/kotlin/com/apollographql/apollo3/ast/introspection/IntrospectionSchema.kt
Show resolved
Hide resolved
@@ -4,7 +4,7 @@ plugins { | |||
antlr | |||
id("org.jetbrains.kotlin.jvm") | |||
id("apollo.library") | |||
id("com.google.devtools.ksp") | |||
kotlin("plugin.serialization") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit. We're using the id("")
notation everywhere else
kotlin("plugin.serialization") | |
id("org.jetbrains.kotlin.plugin.serialization") |
...pollo-ast/src/main/kotlin/com/apollographql/apollo3/ast/introspection/IntrospectionSchema.kt
Show resolved
Hide resolved
} | ||
} | ||
|
||
@OptIn(ExperimentalSerializationApi::class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's ok to use this. If a future version changes the signatures, they will break at runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we bundle and relocate the code, so we should be immune to changes? (OTOH I'm ok with avoiding using this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We relocate in apollo-gradle-plugin
but some users might use apollo-ast
outside the Gradle context so I'd feel safer avoid this for the time being.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense 👍
...o-ast/src/main/kotlin/com/apollographql/apollo3/ast/introspection/schema_to_introspection.kt
Show resolved
Hide resolved
…tion and R8 rules. Also avoid okio calls that are marked unstable.
@@ -14,6 +14,7 @@ pluginManagement { | |||
includeModule("me.champeau.gradle", "japicmp-gradle-plugin") | |||
includeModule("com.gradle.publish", "plugin-publish-plugin") | |||
includeModule("com.github.ben-manes", "gradle-versions-plugin") | |||
includeModule("org.jetbrains.kotlin.plugin.serialization", "org.jetbrains.kotlin.plugin.serialization.gradle.plugin") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed that one. Do we need this? The serialization plugin should be on maven central these days: https://repo.maven.apache.org/maven2/org/jetbrains/kotlin/plugin/serialization/org.jetbrains.kotlin.plugin.serialization.gradle.plugin/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it again in https://github.com/apollographql/apollo-kotlin/pull/4531/files, see what breaks 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version 1.6.10 (which we use during idea sync) is not on Maven central, but now I'm no longer sure this is needed because I removed it on my machine and it still works 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you have configuration cache enabled 😅 . Ok I'll add it again in #4531 with a comment to remove it at some point
com.apollographql.apollo3.relocated
to avoid clashes when using v2 in the same project (related to generateApolloSources fails with NoSuchMethodError after 3.6.2 -> 3.7.1 update #4523)kotlinx.serialization
inapollo-ast
instead ofmoshi
to be able to relocateapollo-ast
fully while not dropping the adapters