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

chore: make ID type strict #13

Merged
merged 2 commits into from
Jun 2, 2020
Merged

chore: make ID type strict #13

merged 2 commits into from
Jun 2, 2020

Conversation

FedericoBiccheddu
Copy link
Contributor

@FedericoBiccheddu FedericoBiccheddu commented May 30, 2020

Closes #11

I think this should considered as a breaking change because TS gives error when using t.ID and the field type is only string or number.

Argument of type 'string | number' is not assignable to parameter of type 'string'.
Type 'number' is not assignable to type 'string'.ts(2345)

@n1ru4l
Copy link
Collaborator

n1ru4l commented May 31, 2020

Internally, the value passed to the resolver for the ID is always cast to a string.

The ID scalar type represents a unique identifier, often used to refetch an object or as the key for a cache. The ID type is serialized in the same way as a String; however, it is not intended to be human‐readable. While it is often numeric, it should always serialize as a String.

(https://spec.graphql.org/June2018/#sec-ID)

That means we have to differentiate between IDs as arguments/inputObject type fields (should be a string) and IDs as resolver return values (should be a string or number).

@FedericoBiccheddu
Copy link
Contributor Author

Good catch!

This makes me think if t.arg(t.IDInt) can lead to confusion due to the ID coercion in resolve.
Should we update resolve signature and infer ID args as string?

Additional references for who is interested:
- ID scalar definition
- ID scalar definition test
- ID serialization test

@FedericoBiccheddu
Copy link
Contributor Author

I did some experiments with ID as string and here are my thoughts.

  1. Having ID as string reflect the original GraphQL behavior even in userland
  2. Reduce overhead and boilerplate due to working with one type (no type guards, explicit casts, etc)
  3. Other libraries treat ID as string only (eg graphql-code-generator, @nexus/schema)

I can update the PR to reflect these considerations.

@sikanhe
Copy link
Owner

sikanhe commented Jun 2, 2020

Good catch!

@sikanhe sikanhe merged commit c6bdace into sikanhe:master Jun 2, 2020
@FedericoBiccheddu FedericoBiccheddu deleted the fb/11-improve-id-type branch June 2, 2020 18:56
@sikanhe sikanhe mentioned this pull request Feb 8, 2021
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.

t.ID should be string | number, not any
3 participants