apollographql / apollo-ios

📱  A strongly-typed, caching GraphQL client for iOS, written in Swift.
https://www.apollographql.com/docs/ios/
MIT License
3.89k stars 728 forks source link

ID as a custom scalar #3379

Closed TizianoCoroneo closed 5 months ago

TizianoCoroneo commented 6 months ago

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
    }
}
AnthonyMDev commented 6 months ago

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!

calvincestari commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

TizianoCoroneo commented 6 months ago

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

github-actions[bot] commented 5 months ago

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.