Fix parsing of default values in buildASTSchema, extendSchema and buildClientSchema#903
Fix parsing of default values in buildASTSchema, extendSchema and buildClientSchema#903IvanGoncharov wants to merge 3 commits intographql:masterfrom
buildASTSchema, extendSchema and buildClientSchema#903Conversation
| * Create a JavaScript value from a GraphQL languate AST representation | ||
| * of Scalar. | ||
| */ | ||
| export function scalarValueFromAST(astValue: ValueNode): mixed { |
There was a problem hiding this comment.
This looks very similar to valueFromAST - can that be used instead?
There was a problem hiding this comment.
If this function is for scalars, then it should likely not support LIST or OBJECT, correct?
There was a problem hiding this comment.
This looks very similar to valueFromAST - can that be used instead?
valueFromAST builds value based on the type definition it accepts as the second argument.
In a case of a scalar, there is no such type definition.
If this function is for scalars, then it should likely not support LIST or OBJECT, correct?
AFAIK, the spec doesn't explicitly forbid complex scalars as LIST or OBJECT. Moreover, it's a widespread practice to create such scalars. More details in this PR:
graphql/graphql-spec#325
| parseValue(value: mixed): mixed { | ||
| const parser = this._scalarConfig.parseValue; | ||
| return parser && !isNullish(value) ? parser(value) : undefined; | ||
| return parser && !isNullish(value) ? parser(value) : value; |
There was a problem hiding this comment.
If a value parses as nullish, shouldn't it return clearly as undefined to not be confused with a true null value?
| // Create a GraphQL language AST from a JavaScript value. | ||
| export { astFromValue } from './astFromValue'; | ||
|
|
||
| // Create a JavaScript value from a GraphQL languate AST representation |
IvanGoncharov
force-pushed
the
astValueToData
branch
2 times, most recently
from
8534882 to
f9bcc32
Compare
IvanGoncharov
force-pushed
the
astValueToData
branch
from
f9bcc32 to
78e69d6
Compare
|
Thanks for making this happen, Ivan. Great improvement |
Closes #903 commit 0062abf Author: Lee Byron <[email protected]> Date: Fri Dec 1 12:49:38 2017 -0800 Amends this PR with a few additions: * Generalizes building a value from an AST, since "scalar" could be misleading, and supporting variable values within custom scalar literals can be valuable. * Replaces isNullish with isInvalid since `null` is a meaningful value as a result of literal parsing. * Exports this new utility from the top level commit 0ca6cf0 Merge: 37717b8 78e69d6 Author: Lee Byron <[email protected]> Date: Fri Dec 1 11:57:55 2017 -0800 Merge branch 'astValueToData' of https://github.com/APIs-guru/graphql-js into APIs-guru-astValueToData commit 78e69d6 Author: Ivan Goncharov <[email protected]> Date: Sun Jun 11 01:07:47 2017 +0300 Fix 'serialize' stub used in buildASTSchema commit a0688c6 Author: Ivan Goncharov <[email protected]> Date: Fri Jun 9 23:24:55 2017 +0300 Provide reasonable default version of 'parseLiteral' commit 313d66e Author: Ivan Goncharov <[email protected]> Date: Fri Jun 9 23:22:49 2017 +0300 Add test for building client schema with default value on custom scalar
3 participants
In the current implementation
buildASTSchema,extendSchemaandbuildClientSchemaregister following stubs asparseLiteralandparseValue:Because of that,
graphql-jscan't correctly parse default values of custom scalar types:So to clarify. You can override those functions after the schema is built, but the default value from IDL will be lost.
To illustrate this I've added a test case in the first commit of this PR and here is the result:
This PR solves this problem by providing reasonable defaults for
parseLiteralandparseValue.