ID Scalar is incorrectly typed

[n1ru4l]

Describe the bug

When you return a number as an ID type in a resolver, it causes a typescript error. However, graphql.js does also accept returning numbers which are then automatically cast to strings.

See https://graphql.github.io/graphql-spec/draft/#sec-ID

TL;DR:

ID as operation argument/return type from resolver -> string or number
ID as operation selection set value -> string

[ardatan]

@n1ru4l I think it is correct right now because if you have a different type than string in your entity(parent) type, you can use mappers to define it like below;

export interface UserEntity {
    id: number;
   name: string;
}

type User {
  id: ID
  name: String
}

export const Query: QueryResolvers = {
   user(_, { id }) => ({ id: 2, name: 'TestUser' }), // Return type is `UserEntity` not `User`
}

mappers: 
    User: ./entities/UserEntity#UserEntity

So in that case, it should be fine, because if we change ID to string | number, it wouldn't be consistent with client which is always string.

[n1ru4l]

So in that case, it should be fine, because if we change ID to string | number, it wouldn't be consistent with client which is always string.

Wouldn't it be possible to distinguish between resolver return ID and client-consumed ID?

[ardatan]

@n1ru4l We can do that but we use those generated types in client as well. But I think they shouldn't be used as parent types because the data coming from another source is generally different than the one sent to the client. I think we should keep that and you should use mappers as I said.
@dotansimha What do you think?

[dotansimha]

I think we can generate different types for client and server, that shouldn't be a problem.
I think the codegen assumes that ID is string in GraphQL, but the actual real value is a number or a string. So I think the should maybe change it in the codegen, even if it's a breaking change for some developers (because it caused by an actual bug in the codegen).

[dotansimha]

The actual default value of the ID scalar should be string | number, to match the implementation of the scalar in graphql.js: https://github.com/graphql/graphql-js/blob/730f5cc6e0c7b7994cb11fe1a9785700fa4dc5db/src/type/scalars.js#L256

@n1ru4l @ardatan @kamilkisiela what do you think? Do you think it will cause any breaking changes?

[n1ru4l]

https://github.com/graphql/graphql-js/blob/730f5cc6e0c7b7994cb11fe1a9785700fa4dc5db/src/type/scalars.js#L239-L247

ID as Input -> String or Number
ID as Output -> String

[dotansimha]

@n1ru4l you are right. I guess the best solution for that is to add inputScalars config and have a default ID = number | string there.

[dotansimha]

Related: #2684