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

Escape "type" in enums and sealed classes #4144

Merged
merged 6 commits into from
Jun 1, 2022
Merged

Escape "type" in enums and sealed classes #4144

merged 6 commits into from
Jun 1, 2022

Conversation

BoD
Copy link
Contributor

@BoD BoD commented May 27, 2022

Escape type in enums and sealed classes, because it clashes with the type: EnumType constant.

This is the easy fix, but may be annoying (expecting to use .type in the client code, and realizing you have to use .type_ instead).

Maybe we could instead rename the type constant (to __type for instance), but type is used when mapping scalars so doing so globally would break client code. Instead maybe we could rename it but only for enums and sealed classes, but then it's not consistent. Instead we could rename it globally but keep a deprecated type on scalars that points to __type. Not sure if there would be other consequences I'm not seeing :) LMK what you think.

Related to #4023 (comment)

@netlify
Copy link

netlify bot commented May 27, 2022

Deploy Preview for apollo-android-docs canceled.

Name Link
🔨 Latest commit e7674f6
🔍 Latest deploy log https://app.netlify.com/sites/apollo-android-docs/deploys/629730a1dccb9700089cdd05

@@ -41,7 +42,7 @@ class SchemaBuilder(
private fun typesFieldSpec(): FieldSpec {
val allTypenames = interfaces.map { it.name } + objects.map { it.name } + unions.map { it.name }
val initilizer = allTypenames.sortedBy { it }.map {
CodeBlock.of("$T.type", context.resolver.resolveSchemaType(it))
CodeBlock.of("$T.$type", context.resolver.resolveSchemaType(it))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Unrelated to this fix, but noticed we didn't use the constant here but we should)

@martinbonnin
Copy link
Contributor

martinbonnin commented May 30, 2022

Maybe we could instead rename the type constant (to __type for instance), but type is used when mapping scalars so doing so globally would break client code.

Yea, that'd have been nice but quite hard to do now

Instead we could rename it globally but keep a deprecated type on scalars that points to __type.

Certainly the path forward. Let's do this as a transition helper for 4.0?

My favorite short term solution would be throw an error and introduce a @targetName(name: String!) directive to customize the kotlin name from graphql:

# extra.graphqls
extend enum Foo {
  bar,
  type @targetName(name: "type_")
}

That'd make a more general solution if we bump into other situations like this.
This could also be leverage by users who want to prefix all their Kotlin classes, using a schema transform Gradle task or so...

For posterity, I also asked the question on the kotlinlang slack

@BoD
Copy link
Contributor Author

BoD commented May 30, 2022

Reverted escaping the enum value type, and renamed constant typeto __type + kept deprecated type on scalars for compatibility.

@@ -30,6 +30,21 @@ fun List<GQLDirective>.findExperimentalReason() = firstOrNull { it.name == "expe
?: "Experimental"
}

fun List<GQLDirective>.findTargetName() = firstOrNull { it.name == "experimental_targetName" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this @ApolloInternal? (I don't think it can be internal?)

Historically the API in apollo-ast isn't super well tracked but I'd like to start moving forward

@@ -24,3 +24,5 @@ fun String.escapeKotlinReservedEnumValueNames() : String {
else -> this
}
}

fun String.isApolloReservedEnumValueName() = this == "type"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here... Maybe we should just start tracking the public APIs everywhere?

See

ignoredProjects.addAll(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! ✅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Contributor

@martinbonnin martinbonnin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@BoD BoD merged commit 8345c91 into main Jun 1, 2022
@BoD BoD deleted the escape-type-enum-name branch June 1, 2022 14:08
@martinbonnin martinbonnin mentioned this pull request Jun 6, 2022
33 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants