-
Notifications
You must be signed in to change notification settings - Fork 2k
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
language support for NullValue #544
Changes from all commits
c1ca9a8
24d3de2
48a463d
c5a24f3
61385b7
9a571ef
8261a4d
588dd43
e60b785
f2c3c20
b1339cf
1c29b5d
07ac6d7
2a28c0c
6b61bb6
bf83f7f
5a3585f
2823bd3
8ffbfef
d5e104f
4803be7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
/* @flow */ | ||
/** | ||
* Copyright (c) 2015, Facebook, Inc. | ||
* All rights reserved. | ||
* | ||
* This source code is licensed under the BSD-style license found in the | ||
* LICENSE file in the root directory of this source tree. An additional grant | ||
* of patent rights can be found in the PATENTS file in the same directory. | ||
*/ | ||
|
||
/** | ||
* Returns true if a value is undefined, or NaN. | ||
*/ | ||
export default function isInvalid(value: mixed): boolean { | ||
return value === undefined || value !== value; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ import { | |
GraphQLString, | ||
GraphQLBoolean, | ||
GraphQLID, | ||
GraphQLNonNull, | ||
} from '../../type'; | ||
|
||
|
||
|
@@ -33,17 +34,26 @@ describe('astFromValue', () => { | |
{ kind: 'BooleanValue', value: false } | ||
); | ||
|
||
expect(astFromValue(null, GraphQLBoolean)).to.deep.equal( | ||
expect(astFromValue(undefined, GraphQLBoolean)).to.deep.equal( | ||
null | ||
); | ||
|
||
expect(astFromValue(null, GraphQLBoolean)).to.deep.equal( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you also add a test for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @leebyron I'm little confused by this. In
So it is correct to ignore this comment here? But I think you are right.. so I should drop this comment.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, it makes sense. I added condition and test in 02d71a0 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah - I like the change you made. |
||
{ kind: 'NullValue' } | ||
); | ||
|
||
expect(astFromValue(0, GraphQLBoolean)).to.deep.equal( | ||
{ kind: 'BooleanValue', value: false } | ||
); | ||
|
||
expect(astFromValue(1, GraphQLBoolean)).to.deep.equal( | ||
{ kind: 'BooleanValue', value: true } | ||
); | ||
|
||
const NonNullBoolean = new GraphQLNonNull(GraphQLBoolean); | ||
expect(astFromValue(0, NonNullBoolean)).to.deep.equal( | ||
{ kind: 'BooleanValue', value: false } | ||
); | ||
}); | ||
|
||
it('converts Int values to Int ASTs', () => { | ||
|
@@ -105,6 +115,10 @@ describe('astFromValue', () => { | |
); | ||
|
||
expect(astFromValue(null, GraphQLString)).to.deep.equal( | ||
{ kind: 'NullValue' } | ||
); | ||
|
||
expect(astFromValue(undefined, GraphQLString)).to.deep.equal( | ||
null | ||
); | ||
}); | ||
|
@@ -133,6 +147,17 @@ describe('astFromValue', () => { | |
); | ||
|
||
expect(astFromValue(null, GraphQLID)).to.deep.equal( | ||
{ kind: 'NullValue' } | ||
); | ||
|
||
expect(astFromValue(undefined, GraphQLID)).to.deep.equal( | ||
null | ||
); | ||
}); | ||
|
||
it('does not converts NonNull values to NullValue', () => { | ||
const NonNullBoolean = new GraphQLNonNull(GraphQLBoolean); | ||
expect(astFromValue(null, NonNullBoolean)).to.deep.equal( | ||
null | ||
); | ||
}); | ||
|
@@ -220,4 +245,26 @@ describe('astFromValue', () => { | |
value: { kind: 'EnumValue', value: 'HELLO' } } ] } | ||
); | ||
}); | ||
|
||
it('converts input objects with explicit nulls', () => { | ||
const inputObj = new GraphQLInputObjectType({ | ||
name: 'MyInputObj', | ||
fields: { | ||
foo: { type: GraphQLFloat }, | ||
bar: { type: myEnum }, | ||
} | ||
}); | ||
|
||
expect(astFromValue( | ||
{ foo: null }, | ||
inputObj | ||
)).to.deep.equal( | ||
{ kind: 'ObjectValue', | ||
fields: [ | ||
{ kind: 'ObjectField', | ||
name: { kind: 'Name', value: 'foo' }, | ||
value: { kind: 'NullValue' } } ] } | ||
); | ||
}); | ||
|
||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might have been discussed elsewhere, but I'm confused why we want to support
null
as a default value.How is this different from:
Except that one will have
args["argument"]
set tonull
and the otherargs["argument"]
will be undefined when omitted.cc @rmosolgo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... Maybe to distinguish between "user wants to set this to
null
" and "user wants to leave this unchanged"?For a Ruby example, the difference is, in the first case:
(Oops, first time I saw this, I thought this was a variable default value of
null
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so by setting the default value of an input argument to
null
you're making it impossible to know if the user passed innull
vs the user omitted the input value entirely.In
six
, the value ofargument
could benull | Int | undefined
.In
seven
, the value ofargument
could benull | Int
.Since both
null
andundefined
are falsy, I can't think of a case where one would explicitly set the default value tonull
in order to omit theundefined
case.Maybe when using Flow/TypeScript removing this 3rd case makes it easier to understand the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default value will be present inside
resolve
Nullable type without value (and without default value) will be omitted ===
undefined
RFC
CC @cjoudrey @rmosolgo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cjoudrey
It is valid language construct. I doesn't care if it is semantically useful - but someone may want explicitly tell that there is no semantical difference between undefined and null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is probably confusing as an example because I agree that it's a little strange to make a default value of
null
to rule out the "not provided" (aka undefined) case, but it's certainly possible now, so it's worth testing as an edge case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default
NULL
value is super useful for me. If I persist document (for MongoDB) withnull
values (preallocate space for field), it saves me from moving document on disk when this value changes (add field and value).