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

ID as a custom scalar #3379

Closed
TizianoCoroneo opened this issue May 13, 2024 · 6 comments · Fixed by apollographql/apollo-ios-dev#363
Closed

ID as a custom scalar #3379

TizianoCoroneo opened this issue May 13, 2024 · 6 comments · Fixed by apollographql/apollo-ios-dev#363
Assignees
Labels
feature New addition or enhancement to existing solutions good first issue Issues that are suitable for first-time contributors.

Comments

@TizianoCoroneo
Copy link
Contributor

Use case

I'd like to be able to customize the backing type of the ID of the codegen-generated models.

In our API we have IDs that are encoded representations of other things (can't go in too much details), so I would like to have my own custom String-wrapper type with additional functionality, instead of the mandated String as the type behind the ID typealias. Today I have to extend String, polluting the namespace instead.

Example: the SchemaMetadata.graphl.swift contains this code:

// @generated
// This file was automatically generated and should not be edited.

import ApolloAPI

public protocol API_SelectionSet: ApolloAPI.SelectionSet & ApolloAPI.RootSelectionSet
where Schema == API.SchemaMetadata {}

/* ... */

public extension API {
  typealias ID = String // <---------- I want to customize this!

Describe the solution you'd like

It would be nice to have it as another custom scalar instead:

public extension API {
    /// Represents an ISO 4217 currency code. For example: EUR, USD, GBP, etc.
    typealias ID = MyCustomID
}

struct MyCustomID: Hashable, CustomScalarType {
    let value: String

    init(_jsonValue value: JSONValue) throws {
        guard let string = value as? String else {
            throw JSONDecodingError.couldNotConvert(value: value, to: String.self)
        }

        self.value = string
    }

    var _jsonValue: JSONValue {
        self.value
    }
}
@TizianoCoroneo TizianoCoroneo added the feature New addition or enhancement to existing solutions label May 13, 2024
@AnthonyMDev
Copy link
Contributor

Hey @TizianoCoroneo! I thought I was going to have to say no to this request (because ID is reserved as a specific type in the GraphQL Spec), but after reviewing the spec, it just says that ID must be serialized to a String. So I think this is totally acceptable!

@calvincestari @BobaFetters do you see any reason why we shouldn't allow this?

If you want to make a PR for this, I think we can accept it!

@AnthonyMDev AnthonyMDev added the good first issue Issues that are suitable for first-time contributors. label May 13, 2024
@calvincestari
Copy link
Member

We can't allow complete customization of the type since it will still need to be bound by the serializable-to-string requirement in the GraphQL spec. I don't know exactly how this would work yet but it'd need to conform to ExpressibleByStringLiteral and CustomStringConvertible, or something like that. There is also a ton of helper extensions that we've got on the basic scalar types already that String benefits from, so it could be complicated to untangle.

Happy to work with you in figuring this out though @TizianoCoroneo.

@AnthonyMDev
Copy link
Contributor

We can't allow complete customization of the type since it will still need to be bound by the serializable-to-string requirement in the GraphQL spec.

I'm not sure how much that matters? Client devs are going to implement their custom scalars to consume the scalar specified by their schema. If your schema is trying to use the ID type as something that isn't serializable to a String, your server itself isn't spec-compliant. But honestly, in that case, is there any value in apollo-ios refusing to work?

There is also a ton of helper extensions that we've got on the basic scalar types already that String benefits from, so it could be complicated to untangle.

I'm not sure if any of these are relevant. ID really does function just as any other custom scalar would. If your ID scalar is just a String, then you're good to go. If not, this proposal would allow you to implement the CustomScalarType protocol and then you would get all the required functionality that a Custom Scalar needs to work.

Maybe I'm missing something, but if so, I think we need to be more clear on what specifically would go wrong with this. I can't actually come up with a situation in which this would be problematic.

@TizianoCoroneo, we aren't going to allocate time to implement this ourselves in the near future, but if it's something you want, please go ahead and try it out. If you get it working in your project and that doesn't reveal any problems that we had missed, we'll do a little more thinking and investigation, but I'm hoping it should be mergeable.

@AnthonyMDev
Copy link
Contributor

Implemented in #363. Should be included in next release!

@TizianoCoroneo
Copy link
Contributor Author

Hey 👋
I had a half baked implementation of this, but you were quicker 😂
Thanks for the fast response!

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 iOS 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
Labels
feature New addition or enhancement to existing solutions good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants