Scalar coercion cleanup #1414
Scalar coercion cleanup #1414
Conversation
IvanGoncharov
force-pushed
the
scalarCoercionCleanup
branch
from
9f803fe to
e5f4c44
Compare
| it('serializes output as Boolean', () => { | ||
| expect(GraphQLBoolean.serialize('string')).to.equal(true); | ||
| expect(GraphQLBoolean.serialize('false')).to.equal(true); | ||
| expect(GraphQLBoolean.serialize('')).to.equal(false); |
There was a problem hiding this comment.
Converting non-empty strings to true is confusing because GraphQLString coerce true into "true". Also, spec doesn't mention this coercion:
http://facebook.github.io/graphql/June2018/#sec-Boolean
| function article(id) { | ||
| return { | ||
| id, | ||
| isPublished: 'true', |
There was a problem hiding this comment.
one more example why current coercion rules looks strange and confusing:
"true" => true
"false" => true
IvanGoncharov
added
the
PR: breaking change 💥
| ); | ||
|
|
||
| expect(() => GraphQLID.serialize(-1.1)).to.throw( | ||
| 'ID cannot represent value: -1.1', |
There was a problem hiding this comment.
Oh that's a great change: making the test clear the reason it fails is floating-point coercion not negative-number coercion.
| throw new TypeError( | ||
| `Int cannot represent an array value: ${inspect(value)}`, | ||
| ); | ||
| let num = value; |
There was a problem hiding this comment.
I feel like this logic could be simplified. It's really strange to me to have an early-return in an else block:
if (typeof value === 'boolean') {
return value ? 1 : 0;
}
const num = (typeof value === 'string' && value !== '') ?
Number(value) :
value;
if (!isInteger(num)) {
| throw new TypeError( | ||
| `Float cannot represent an array value: ${inspect(value)}`, | ||
| ); | ||
| let num = value; |
IvanGoncharov
force-pushed
the
scalarCoercionCleanup
branch
from
e5f4c44 to
7f631d2
Compare
@mjmahone Thanks for review 👍 Fixed 🔧 |
3 participants
Idea is to make coercion code more strict and explicit.