-
Notifications
You must be signed in to change notification settings - Fork 396
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
[discussion] Specify TypeScript types for custom scalars #1248
Comments
This is something we definitely want to add to the TS definitions generator at some point. If you would like to create a PR for this, that would be awesome! |
I'd love to take a look at a PR for this, after I meet some looming deadlines... |
After exploring the code a bit, here's my proposal:
export interface DefinitionsGeneratorOptions {
emitTypenameField?: boolean;
skipResolverArgs?: boolean;
defaultScalarType?: string; // defaults to 'any' for backwards compatibility
scalarDefinitions?: Record<string, string>;
}
I think this can be done in a non-breaking way, and in fact, the PR shouldn't be too onerous. Thoughts/comments? |
One other feature that this will require is the ability to add a "header" to the generated I think this could be served by another optional string key |
Let's track this here #1253 |
Feature Request.
I'm having the same issue as #540, but I'd like to take my own stab at explaining it, and why the resolution of this issue is unsatisfactory.
I'm a big fan of TypeScript and having consistently applied, strong types throughout my server code. To support that, I'm using https://github.com/urigo/graphql-scalars for more specialized scalars, to support custom scalar types like BigInt, DateTime etc.
I've defined application-custom scalars in
base.graphql
:And have supplied appropriate resolvers for them in the GraphQL module, etc. This all works fine, and GraphQL correctly serializes and deserializes the scalar values.
However, the generated TypeScript types are insufficient:
Even though I've defined my DateTime resolver to parse to/serialize from a JS
Date
object, I'm unable to leverage that knowledge - that my application is in full control of - to know that a GraphQL argument of typeDateTime
is actually aDate
. Instead, I have a bunch ofany
values. And in my application, I go to pains to avoid the use ofany
.So first of all, a more appropriate default "unknown" type would be TypeScript 3.0's 'unknown`. This would force explicit (rather than implicit) casting, and require assumptions to be made clear.
But secondly, like the author of the original ticket, I've used
graphql-codegen
and think its model works fairly well as an example of what might be possible in a framework as ambitious as Nest.js.In graphql-codegen, I can manually specify how I want my custom GraphQL scalar values to be mapped to TypeScript like so:
Now I'm able to generate the following TypeScript:
This is the functionality I'm looking for.
Of course, if you want to get technical about it, Nest.js has some shortcomings in terms of its interaction with TypeScript and build-time type safety. For example, as an argument to a resolver query or mutation, I can write:
And the compiler has no way of checking that this is an actual argument specified in any GraphQL
input
type for this function; it has to do all its checking at runtime. Likewise, overriding custom scalars with a compiler mapping like this is really just a "fancy" way to cast what's really anany
; the compiler has to "take my word for it" that these types are correct.The advantage of being able to specify this mapping, however, is that assuming the mapping is correct, any code downstream from the generated types will now be typesafe.
In sum:
any
is essentially the worst possible type that can be generated, and its use in output should be avoided at all costs. Ideally there would be a way to specify a scalar mapping as I've outlined, but at a minimumunknown
should be used in the place ofany
in all generated types.Thanks for your consideration and for all the work you've put into a very solid framework.
The text was updated successfully, but these errors were encountered: