-
Notifications
You must be signed in to change notification settings - Fork 56
Generated interfaces for input types should use undefined instead of null #278
Comments
@cyrus-za Everything looks fine for me : It was not working before, and had been fixed days ago in one of last releases, as said @schickling on #50 :
This is your custom https://github.com/cyrus-za/graphqlgen-example/blob/master/src/types.ts which is not valid and cause the error. You might ask why using GraphQL servers automatically map On the other hand, using And this is what happening in your code here : https://github.com/cyrus-za/graphqlgen-example/blob/master/src/resolvers/Query.ts#L5 The typing mismatching is between :
So you need to replace |
@kevinmarrec We still have an issue with input types though. An optional GraphQL input field can either be undefined (in the sense of typescript), null, or have a value. Given the following schema: input UserInput {
field: String
}
type Query {
users(input: UserInput): [User!]!
} The following queries are all valid: {
users { ... }
} {
users(input: null) { ... }
} {
users(input: { ... }) { ... }
} We need to change the typings for input types and make optional GraphQL fields for input types defined as so: @schickling What do you think? |
@Weakky Alright, I didn't go deep testing GraphqlGen with inputs types yet. Anyway, if I'm right, there is already an opened issue about the input types. The error in |
@kevinmarrec that is exactly the problem. It is one or the other. I am also not aware of the graphql spec saying GraphQL servers automatically map undefined values as null in client responses. |
@cyrus-za I don't know where it says that, but maybe there is an explanation : graphql/graphql-js#731 |
@kevinmarrec that is graphql-js which we are not using. We using a schema file directly. Nowhere in the facebook graphql spec does it say that undefined means null. Neither will it as some languages dont even have a null type. This is an opinionated approach one way or another. I am just of the opinion that if a model interface has optional question mark (which as per typescript documentation means |
Alright then I don't know. All I know is that if your make your resolver return |
That is dependent on your framework. GraphQLjs maybe. Apollo server does
not resolve null into null or undefined. It actually discards the property
completely. Yoga does the same as it is built on top of Apollo.
…On Wed, Nov 14, 2018, 20:07 Kevin Marrec ***@***.***> wrote:
Alright then I don't know.
All I know is that if your make your resolver return undefined, it's
resolved to null in client response 😁
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#278 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABwqZSzLBlUJRQTAVMElWXjflSertkKZks5uvFvBgaJpZM4YX9XS>
.
|
I think there is a misunderstanding. @kevinmarrec is talking about "value types", things the server returns to the client, but @Weakky (and I think @cyrus-za as well) are talking about "input types", things the client sends to the server. As far as I can tell, no one actually disagrees with each other, you're just talking about different things. My 2 cents:
The spec does not say anything about undefined inputs sent to the server being replaced with null. The most it says is
Which implies that there may be a difference between passing null and leaving an input undefined. Given that some JavaScript implementations of GraphQL will pass omitted inputs as undefined, the generated types for inputs should cover this extra case. |
Well said!
I am definitely talking about inputs. The returned value is already defined
in the models. It is the input that is auto generated that is the problem.
Given a returned value of undefined in the interface, it is expected that
the generated input will allow undefined, but it only allows an explicit
null and not a onmited value. The generated js from the ts will actually
allow onmited values but ts does not compile.
…On Thu, Nov 15, 2018, 15:52 Joshua T Shaffer ***@***.***> wrote:
I think there is a misunderstanding. @kevinmarrec
<https://github.com/kevinmarrec> is talking about "value types", things
the server returns to the client, but @Weakky <https://github.com/Weakky>
(and I think @cyrus-za <https://github.com/cyrus-za> as well) are talking
about "input types", things the client sends to the server. As far as I can
tell, no one actually disagrees with each other, you're just talking about
different things.
My 2 cents:
The GraphQL spec only says that undefined values returned to the client
will be replaced with null.
If result is null (or another internal value similar to null such as
undefined or NaN), return null.
The spec does not say anything about undefined inputs sent to the server
being replaced with null. The most it says is
In addition to not accepting the value null, it [a non-nullable input]
also does not accept omission.
Which implies that there may be a difference between passing null and
leaving an input undefined.
Given that some JavaScript implementations of GraphQL will pass omitted
inputs as undefined, the generated types for inputs should cover this extra
case. argName?: argType | null
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#278 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABwqZeoo3521Yyvl_J2smSNwQK3YiYrCks5uvXG4gaJpZM4YX9XS>
.
|
I think this issue shows up when using Prisma with graphqlgen. Prisma generates the following interface:
GraphQlGen does this:
Here's the mutation in schema.graphql
User in datamodel.prisma
After generating the code with prisma generate, followed by graphql get-schema then graphqlgen, I get the error when implementing the resolver for createuser mutation:
Is there a workaround for this (or am I doing something wrong)? Been stuck on this for 2 days =\ |
Hey there, @joshuatshaffer got it right! Here is the full problem: GraphQL does indeed accept The problem is that This means that even if we fix this issue on In order to fix this, we need to both fix @schickling @timsuchanek: What do you think? |
I just found out that the same problem applies to the Flow client as well! I've added a note to the corresponding issue in the |
Hello! const { graphql } = require("graphql")
const { makeExecutableSchema } = require("graphql-tools")
const { Binding } = require("graphql-binding")
const schema = makeExecutableSchema({
typeDefs: `
type Query {
echo(input: [String]): [String]
binding(input: String): [String]
}
`,
resolvers: {
Query: {
echo: (root, { input }) => {
const output = Array.isArray(input)
? input.map(e => (e === null ? undefined : e))
: input
console.log("resolver input", input)
console.log("resolver output", output)
return output
},
binding(root, { input }, { binding }, info) {
console.log("binding input", input)
return binding.query.echo({ input }, info).then(output => {
console.log("binding output", output)
return output
})
}
}
}
})
const root = undefined
const context = {
binding: new Binding({ schema })
}
const queries = [
"{ echo }",
"{ echo(input: null) }",
'{ echo(input: "1") }',
'{ echo(input: ["1", null]) }',
"{ binding }",
"{ binding(input: null) }",
'{ binding(input: "1") }'
]
queries.reduce(
(promise, query) =>
promise.then(_ => {
console.log("query", query)
return graphql(schema, query, root, context).then(result => {
if (result.errors) {
console.error("Err", result.errors.map(e => e.message))
} else {
console.log("Ok", result.data)
}
console.log("---")
})
}),
Promise.resolve()
) Output:
Types: interface BindingArgs {
input: string | null | undefined
}
interface ResolverArgs {
input: Array<string | null> | null | undefined
}
type ResolverReturns = Array<string | null | undefined> | null | undefined
type BindingReturns = Array<string | null> | null For ResolverArgs graphqlgen generates now: interface ResolverArgs {
input: Array<string | null | undefined> | null | undefined
} I have not found a way to pass |
Any update on what to do? For now I'm switching my types to field?: string | null but I don't know if this the the correct solution. |
There is a semantic difference IMHO between setting Using Using An input field in a mutation, that is optional (In other words: that can be ommited), should be |
@nickluger There is definitely a difference between @Weakky Unfortunately, Flow seems unable to make a type that allows both @b33son For now I recommend using |
I've got a script that imports graghqlgen.ts to my client project before tests and using the Mutation ResolverArgs for type safety on an automated way.
This will work great once the interface defines optional fields correctly. |
I have posted a comment with code samples on the prisma end of this thread here prisma/prisma#3621 (comment). The TL;DR for TypeScript the most correct approach I see is almost what @Weakky stated but with the addition of supporting
My PR #329 should take care of point 2 coincidentally. |
Hey everyone, worked a bit on this tonight, moving the existing PR forward. Almost there, except #331 (comment). I think I'll have it done this week, or next. |
The PR also addressed resolver return types, not just resolver params. I'm not sure that was mentioned in this thread so just mentioning it. I chose not to allow |
Description
When a field is optional in the schema, graphqlgen generates the types as
It is expected to rather generate using
undefined
instead ofnull
or the?
shorthand.This issue only occurs when
tsconfig.json
hasstrict: true
which comes withgraphql-boilerplate
.Steps to reproduce
Clone this repo and do a
tsc
build to see the errors.https://github.com/cyrus-za/graphqlgen-example
Expected results
No error.
Actual results
Versions
PS. This is related to #50 but I was asked to create a new issue. @schickling
The text was updated successfully, but these errors were encountered: