-
Notifications
You must be signed in to change notification settings - Fork 2k
language support for NullValue #544
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
Changes from 10 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 null, 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 |
---|---|---|
|
@@ -17,6 +17,7 @@ type Foo implements Bar { | |
four(argument: String = "string"): String | ||
five(argument: [String] = ["string", "string"]): String | ||
six(argument: InputType = {key: "value"}): Type | ||
seven(argument: Int = null): Type | ||
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. This might have been discussed elsewhere, but I'm confused why we want to support How is this different from: seven(argument: Int): Type Except that one will have cc @rmosolgo 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. Hmmm... Maybe to distinguish between "user wants to set this to For a Ruby example, the difference is, in the first case: # seven(argument: Int)
# User provided `null`
args.key?(:argument) # => true
args[:argument] # => nil
# User has not provided a value
args.key?(:argument) # => false
args[:argument] # => nil (this is the same)
# seven(argument: Int = null)
# In this case, the outputs are identical:
# User provided `null`
args.key?(:argument) # => true
args[:argument] # => nil
# User has not provided a value
args.key?(:argument) # => true
args[:argument] # => nil (this is the same) (Oops, first time I saw this, I thought this was a variable default value of 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. Right, so by setting the default value of an input argument to six(argument: Int): Type
seven(argument: Int = null): Type In In Since both 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 commentThe reason will be displayed to describe this comment to others. Learn more. 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.
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 commentThe 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 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. Default |
||
} | ||
|
||
type AnnotatedObject @onObject(arg: "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.
comment is incorrect
Uh oh!
There was an error while loading. Please reload this page.
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.
Good catch! Fixed