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.

Sign up for GitHub

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

Adding config options for custom schema type names #242

Merged
merged 6 commits into from
Feb 6, 2024
Merged

Adding config options for custom schema type names #242

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d0f0807
Adding config options for custom schema type names
BobaFetters Jan 22, 2024
5e4dd2b
Updating SchemaCustomization and enums
BobaFetters Jan 26, 2024
ff25185
Addressing feedback and adding tests
BobaFetters Feb 3, 2024
3af853f
Merge branch 'main' into feature/rename/config-options
BobaFetters Feb 3, 2024
be4a2ed
Merge branch 'feature/rename-schema-types' into feature/rename/config…
BobaFetters Feb 6, 2024
ee0315a
Updating sample json
BobaFetters Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions SampleConfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
{
"schemaNamespace" : "MySchema",
"schemaDownload": {
...
},
"input" : {
...
},
"output" : {
...
},
"options" : {
...
"schemaCustomization" : {
"customTypeNames" : {
"InterfaceName": {
"interface": {
"name": "CustomInterfaceName"
}
},
"ScalarName": {
"customScalar": {
"name": "CustomScalarName"
}
},
"InputObjectName": {
"inputObject": {
"fields": {
"fieldOne": "CustomFieldOne"
},
"name": "CustomInputObjectName"
}
},
"ObjectName": {
"object": {
"name": "CustomObjectName"
}
},
"UnionName": {
"union": {
"name": "CustomUnionName"
}
},
"EnumName": {
"enum": {
"name": "CustomEnumName",
"cases": {
"enumCase": "CustomEnumCase"
}
}
}
}
}
}
}
43 changes: 43 additions & 0 deletions ...nLib/CodegenConfiguration/ApolloCodegenConfiguration+RenameSchemaTypesConfiguration.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import Foundation

struct SchemaCustomization: Codable {
Copy link
Member Author

Choose a reason for hiding this comment

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

Figured we could name this SchemaCustomizations so it could potentially contain more options in the future.


let customTypeNames: [String: CustomSchemaTypeName]

// MARK: - Codable

enum CodingKeys: CodingKey, CaseIterable {
case customTypeNames
}

public init(from decoder: Decoder) throws {
let values = try decoder.container(keyedBy: CodingKeys.self)
try throwIfContainsUnexpectedKey(
container: values,
type: Self.self,
decoder: decoder
)

customTypeNames = try values.decode(
[String: CustomSchemaTypeName].self,
forKey: .customTypeNames
)
}

public func encode(to encoder: Encoder) throws {
var container = encoder.container(keyedBy: CodingKeys.self)

try container.encode(self.customTypeNames, forKey: .customTypeNames)
}

// MARK: - Enums

enum CustomSchemaTypeName: Codable {
Copy link
Member Author

Choose a reason for hiding this comment

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

@AnthonyMDev @calvincestari Wanted to get a draft up of what I was thinking here for the new config option. Thinking its best to have it be more verbose and descriptive, adds a little more to the JSON to do the configuration but makes it very clear what you are doing.

Technically could probably combine the enum for some types into one generic one but keping them separate gives us flexibility going forward to add more options to them individually like the enums or input objects.

Also added a sample of hte config JSON showing what they would look like to decode into this structure (that wont be part of the final commit)

Copy link
Member

Choose a reason for hiding this comment

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

It looks like the user is able to specify a bunch of custom type names with this but where do they specify the old schema type that it's being substituted for?

Copy link
Member Author

Choose a reason for hiding this comment

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

So the CustomSchemaTypeName enum is to give us something better to work with on the swift side vs something like a [String: String] dictionary. So on the Swift side what we end up with in the SchemaCustomizations struct is a [String: CustomSchemaTypeName] dictionary where the String key is the name of the type you are customizing the name for as it exists in the schema, and the enum contains the type of object it is (interface, object, scalar, etc) plus the new name you want, and possibly some other info like changing names of cases on an enum. That way when we are consuming this data on the swift side during code generation we will have the name of a type when we are working with it and we can easily check if there is a customization available for it to use.

Copy link
Member

@calvincestari calvincestari Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

OK, that's fine. The sample JSON didn't seem to reflect this which was confusing.

Do we anticipate that users will need this degree of flexibility for type renaming? i.e.: one type named many different things in different contexts? I'm mostly opposed to this being a configuration option vs. a directive. For a directive I was thinking if it could be a type extension in the operation document:

extend type User @typeRename("MyNewType")

Copy link
Member Author

Choose a reason for hiding this comment

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

Well it would still only allow you to rename a type once, not multiple different times since the type you are renaming is used as the dictionary key.

And yea @AnthonyMDev and I discussed config option vs directive and thought this may be the route to go but we can continue that discussions because the data structure I laid out and the further implementation of it will remain the same either way, just how it gets configured would change.

Copy link
Member

Choose a reason for hiding this comment

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

Well it would still only allow you to rename a type once, not multiple different times since the type you are renaming is used as the dictionary key.

Yes I see that now, my mistake. I'm really not a fan of the JSON configuration, it's too messy compared to a single directive.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm more inclined towards the JSON configuration option because it keeps a clear separation between what is client-side vs the actual schema. In your .graphql files, you will still need to use the existing names of the types and enum cases. They will only have the names changed in the generated Swift code. By using a schema extension, it blurs the lines there and makes it less clear.

That said, I think the JSON config format needs to be cleaned up a bit, as this first pass at is is pretty complex and makes actually configuring these things really frustrating and error prone. I'll be posting my opinions on that format in my review shortly.

Copy link
Member

@calvincestari calvincestari Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In your .graphql files, you will still need to use the existing names of the types and enum cases.

I agree, having to use the 'old' type name in operations alongside the rename directive is confusing. I think we could probably twist our frontend to allow the 'new' name everywhere but is that more or less desirable.

Configuring this via JSON still doesn't resolve the problem of needing to use old types in operations but new types in Swift. It's still just as confusing and the JSON config usually doesn't form part of the project source so it's a buried change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could probably twist our frontend to allow the 'new' name everywhere but is that more or less desirable.

I think that is less desirable.

Configuring this via JSON still doesn't resolve the problem of needing to use old types in operations but new types in Swift.

I agree with you that it's still a bit confusing having to use the old names in your GraphQL files and the new names in your Swift files, but I think it at least provides a clear line of delineation. You are still writing GraphQL that is valid against your schema. In all places where you are writing GraphQL, you are using the names provided by your schema.

The alternatives would be to either:

  • Allow them to use either the new or old name in the graphql
    • This could be even more confusing and adds a lot of complexity
    • We would need to do more transforms of those types to send the old type names in the actual network requests
  • Require them to use the new names in the graphql
    • This would also be confusing, as you are only using the new name for types in which you have overridden the names. Types that haven't got a custom name will be the same as they are in your schema.
    • People use their schema and schema docs as a reference when writing their queries. They could also share queries across multiple platforms.

It's still just as confusing and the JSON config usually doesn't form part of the project source so it's a buried change.

This would still be the case with the directives unfortunately. They would just be buried in a schema extension .graphqls file. Because this is changing the names of types for the entire schema, not an individual operation definition, there is no way for this to really exist locally.

I think it's appropriate for this feature to only affect the generated Swift code. And if it only affects the Swift code, I think it's appropriate to keep it as a config option rather than a schema directive. It's never going to be 100% obvious what's going on when you use these custom names. It's not like field aliases that are local to the operation that declares them. These are sweeping overall changes to your schema.

case object(name: String)
case interface(name: String)
case customScalar(name: String)
case union(name: String)
case `enum`(name: String, cases: [String: String])
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might want to make name an optional String? on this and inputObject. You may want to change the name of cases/fields without changing the name of the type itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree ease of use is important, but I also think in general some extra complexity for more explicit clarity of what is happening can be useful as well, in this case both from the JSON side and especially having the different enum cases on the swift side in case there may be times that information is needed. I think a setup like that is a nice middle ground between just strings as you suggested, and having a directive which would make it very clear what type was being modified. That said, I'm happy to make the suggested changes and focus on ease of use.

Also yea the name will need to be optional for the scenario you described.

case inputObject(name: String, fields: [String: String])
}
}
Loading