language support for NullValue #544

langpavel · 2016-10-27T10:54:18Z

GraphQL RFC: Null value

Support null literal in Parser
Support null literal in Printer
astFromValue
valueFromAST
getArgumentValues, getVariableValue in execution/values.js
isValidLiteralValue in utilities/isValidLiteralValue.js
introspection of __InputValue defaultValue field in type/introspection.js
utilities/schemaPrinter.js
some more tests

langpavel · 2016-10-27T16:22:11Z

Help wanted! 😄

leebyron

Thanks for starting this!

leebyron · 2016-10-27T20:43:00Z

src/language/ast.js

+export type NullValue = {
+  kind: 'NullValue';
+  loc?: Location;
+  // value: null;


no need for this line - I think it's more confusing than helpful since the type doesn't actually have a value field, and if you attempted to read one, you'd see undefined instead of null

I was in doubt with this. Can I add value property? Because other literal AST nodes have value - so I keep it commented. So you suggest remove value or actually set it to null?

I think remove it will be better, but thank you for your careful review 👍

I agree, removing it is better. No sense in including the field which will always be the same value :)

leebyron · 2016-10-27T20:47:44Z

src/utilities/__tests__/astFromValue-test.js

      null
    );

+    expect(astFromValue(null, GraphQLBoolean)).to.deep.equal(


Could you also add a test for astFromValue(null, new GraphQLNonNull(GraphQLBoolean)) to expect null

@leebyron I'm little confused by this.
What is correct result for astFromValue(null, new GraphQLNonNull(*))?

In astFromValue implementation there is this comment at line 74:

Note: we're not checking that the result is non-null.
This function is not responsible for validating the input value.

So it is correct to ignore this comment here? But I think you are right.. so I should drop this comment..

Ok, it makes sense. I added condition and test in 02d71a0

Yeah - I like the change you made.

leebyron · 2016-10-27T20:48:26Z

src/utilities/astFromValue.js

+  // only explicit null, not undefined, NaN
+  if (_value === null) {
+    return ({ kind: NULL }: NullValue);
+  } else if (isNullish(_value)) {


no need for the else since you're returning in the if branch

langpavel · 2016-10-28T02:53:04Z

@leebyron Can you review please?

I think I'm done with code, if you want some more tests I can add, but it seems I shift coverage a little up and I'm happy with mine 🔢 😄

leebyron

This is looking great!

Also take a look at getArgumentValues and getVariableValue in execution/values.js - they have a similar kind of isNullish check to determine whether or not to apply a default value. Right now those are probably behaving correctly between null and undefined values. Adding tests for ensuring default values are used for variables definitions and argument definitions in those conditions would be great.

Also check out isValidLiteralValue in utilities/isValidLiteralValue.js - that also needs to handle the null literal. It's used during validation for DefaultValuesOfCorrectType and ArgumentsOfCorrectType - it would be great to add new test cases for those two (in validation/tests/) for nullable and non-null types being provided a null literal.

Also check out the introspection of __InputValue defaultValue field in type/introspection.js - right now it probably fails to represent a default value of null (which would appear as "null", representing null in a string of the graphql language)

Finally there's utilities/schemaPrinter.js which needs to support null literals as default values.

(I found the above by grepping for usage of isNullish - then checking to see if adding null literal support would break its behavior)

leebyron · 2016-10-28T03:05:32Z

src/utilities/valueFromAST.js

        obj[fieldName] = fieldValue;
+      } else if (fieldValue !== null) {


This could just be else since fieldValue !== null is guaranteed to be true here, otherwise the first branch would have been followed.

Also, the two conditions above are the same.

I think you can further shorten to a trinary.

// If no valid field value was provided, use the default value obj[fieldName] = fieldValue === undefined || fieldValue !== fieldValue ? field.defaultValue : fieldValue;

I think you can probably skip the isNullish in favor of inlining the check here. I think isNullish(x) && x !== null would be confusing to read.

Ternary it is, thanks

leebyron · 2016-10-28T03:15:24Z

src/utilities/valueFromAST.js

@@ -111,6 +117,7 @@ export function valueFromAST(
  );

  const parsed = type.parseLiteral(valueAST);
+  // TODO: Should be this condition ommited?


This is a little bit tricky now, but I think the right decision is to leave this check alone.

Right now the definition for parseLiteral is to either return a value, or return null (or some nullish value) if the parse failed. I don't think it's ever appropriate for parseLiteral to return null and have that be intended as a null value to be used.

Comment removed

leebyron · 2016-10-28T03:16:37Z

src/utilities/astFromValue.js

@@ -52,6 +54,7 @@ import { GraphQLID } from '../type/scalars';
 *
 * | JSON Value    | GraphQL Value        |
 * | ------------- | -------------------- |
+ * | null          | NullValue            |


nitpick, could you put this entry at the bottom of the table, that way it's in the same order as the comments above valueFromAST.

leebyron · 2016-10-28T03:17:12Z

src/utilities/__tests__/astFromValue-test.js

      null
    );

+    expect(astFromValue(null, GraphQLBoolean)).to.deep.equal(


Yeah - I like the change you made.

langpavel · 2016-10-28T03:36:37Z

Ahh, sorry, I rebased! But I hope your input was not lost!
I have 5:37 A.M. here so I probably should go sleep today 😄 🌃

leebyron · 2016-10-28T03:37:15Z

Definitely get some rest. Thanks for the progress!

langpavel · 2016-10-28T14:51:41Z

Hi @leebyron. I'm kindly asking you for next review round, please :-)

I still should write some more tests mainly for validating null in NonNull but I'm not sure where is best place to settle this tests. Can you hint me?

Thanks!

langpavel · 2016-10-28T21:03:35Z

I also discovered that it is not completed yet, because it is still possible to set default value to null for NonNull fields... This is definitely not right but I don't know how I can check this correctly..

langpavel · 2016-11-01T00:12:10Z

@leebyron It's finally done I think 😋
(Except of changing behavior of astFromValue — which I think is bad idea)

leebyron · 2016-11-01T00:51:32Z

Which astFromValue change are you referencing?

langpavel · 2016-11-01T00:56:01Z

Which astFromValue change are you referencing?

astFromValue returns null. You proposed undefined but expected result is Value AST node...

Result for null value is ({ kind: NULL }: NullValue) and this function will never returns undefined

leebyron · 2016-11-01T01:01:39Z

Yeah it's a minor detail since you're right that the function either returns an AST node or it does not do so. My concern is that returning null to represent no AST node has the potential to be misunderstood as a representation of null, vs implicitly returning undefined with return; also represents no AST node but is far less likely to be misunderstood.

langpavel · 2016-11-01T01:12:35Z

Yeah it's a minor detail...

Exactly, but it is exported interface covered with tests so I'm for not changing it's behavior..

But if you wish I can change tests and return; from this function.. Then it should be mentioned in changelog too..

langpavel · 2016-11-01T01:53:22Z

Ok,I will go sleep, deep night here :-) Please think about all this.. I know it is big thing so it probably needs more time for landing then I think just now.. But I'm very excited with this null support here..

I know, some people can hate me in future for introducing this null thing again to their lives, but I love possibility to say: Hey system, I don't know this value! :-)

And in this point I must say: Hey @leebyron, thank you for null RFC and I'm really happy I can work on this thing with you!

Good night

Comment clarity

Valid return values

leebyron · 2016-11-01T02:24:10Z

Thanks for all your hard work - let's land this and see how it goes

nodkz · 2016-11-01T10:36:18Z

Can not wait for 0.8.0 anymore ;)
For me, a custom package build with NULL works perfectly.
Thanks @langpavel and @leebyron!

If somebody wants to test NULL in args and flowtype in their app, I built graphql-js package on commit 2b717f1. Just run:

npm install https://www.dropbox.com/s/zrd3b00dmx3jhae/graphql-0.7.2.tgz\?dl\=1

PS. Yarn does not work with this link (((

langpavel · 2016-11-01T11:56:44Z

Awesome! Thanks!

…graphql-js till 0.8.0 More details graphql/graphql-js#544

facebook-github-bot added the CLA Signed label Oct 27, 2016

langpavel mentioned this pull request Oct 27, 2016

[RFC] Null value graphql/graphql-spec#83

Merged

leebyron reviewed Oct 27, 2016

View reviewed changes

langpavel force-pushed the graphql-null-83 branch from b2d6ce2 to 215b9c2 Compare October 28, 2016 02:47

langpavel mentioned this pull request Oct 28, 2016

How to update column value with mutation to NULL graphile/crystal#108

Closed

langpavel force-pushed the graphql-null-83 branch from 215b9c2 to 1ffc489 Compare October 28, 2016 03:29

leebyron requested changes Oct 28, 2016

View reviewed changes

langpavel force-pushed the graphql-null-83 branch from 1ffc489 to 0815e3f Compare October 28, 2016 11:53

langpavel added 2 commits October 28, 2016 14:02

language support for NullValue

c1ca9a8

support null literal in Printer

24d3de2

langpavel force-pushed the graphql-null-83 branch 4 times, most recently from ece2b53 to f747000 Compare October 28, 2016 12:17

langpavel added 5 commits October 28, 2016 14:19

astFromValue returns NullValue for explicit null

48a463d

astFromNode does not converts NonNull values to NullValue

c5a24f3

astFromValue correctly handles NonNull values

61385b7

test: astFromValue converts input objects with explicit nulls

9a571ef

handle null values in valueFromAST

8261a4d

langpavel force-pushed the graphql-null-83 branch 2 times, most recently from 9812d88 to 8261a4d Compare October 28, 2016 13:05

langpavel added 2 commits October 28, 2016 16:02

Support null in schemaPrinter

588dd43

isValidLiteralValue: check for NullValue in NonNull type

e60b785

Add nullish in kitchen sink test

f2c3c20

langpavel added 4 commits November 1, 2016 00:29

a note about difference between undefined and null

bf83f7f

one test for null is enough

5a3585f

be consistent in return

2823bd3

ArgumentsOfCorrectType tests from null values

8ffbfef

leebyron added 2 commits October 31, 2016 19:17

Update valueFromAST.js

d5e104f

Comment clarity

Update valueFromAST.js

4803be7

Valid return values

leebyron approved these changes Nov 1, 2016

View reviewed changes

leebyron merged commit c23f578 into graphql:master Nov 1, 2016

leebyron mentioned this pull request Nov 1, 2016

Enforces input coercion rules. #553

Merged

nodkz mentioned this pull request Nov 1, 2016

Null value in list gets removed by Relay? facebook/relay#1252

Closed

langpavel mentioned this pull request Nov 1, 2016

Support null graphql/codemirror-graphql#51

Merged

langpavel deleted the graphql-null-83 branch November 1, 2016 13:52

calebmer mentioned this pull request Nov 1, 2016

New GraphQL release graphile/crystal#202

Closed

OlegIlyenko mentioned this pull request Nov 1, 2016

Language support for NullValue sangria-graphql/sangria#181

Closed

vladar mentioned this pull request Nov 12, 2016

add support for NullValue webonyx/graphql-php#76

Closed

nodkz added a commit to graphql-compose/graphql-compose that referenced this pull request Nov 14, 2016

feat(Dependencies): Due null improvements highly recommend upgrade …

8da567e

…graphql-js till 0.8.0 More details graphql/graphql-js#544

jnwng mentioned this pull request Feb 16, 2017

Unexpected name "null" when null is provided to an arguments apollographql/graphql-tag#47

Closed

mpoeter mentioned this pull request Jul 12, 2017

Language support for NullValue absinthe-graphql/absinthe#344

Closed

vergenzt mentioned this pull request Jul 26, 2017

GraphQL should allow mutating optional fields to 'null'. #133

Closed

kimhansuk mentioned this pull request Jul 8, 2019

add support for NullValue MichalLytek/type-graphql#372

Closed

jord1e mentioned this pull request Jan 20, 2022

Consider adding defaultValue to @Argument spring-projects/spring-graphql#250

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

language support for NullValue #544

language support for NullValue #544

langpavel commented Oct 27, 2016 •

edited

Loading

langpavel commented Oct 27, 2016 •

edited

Loading

leebyron left a comment

leebyron Oct 27, 2016

langpavel Oct 27, 2016

leebyron Oct 27, 2016

leebyron Oct 27, 2016

langpavel Oct 28, 2016 •

edited

Loading

langpavel Oct 28, 2016

leebyron Oct 28, 2016

leebyron Oct 27, 2016

langpavel commented Oct 28, 2016

leebyron left a comment

leebyron Oct 28, 2016

langpavel Oct 28, 2016

leebyron Oct 28, 2016

langpavel Oct 28, 2016

leebyron Oct 28, 2016

langpavel Oct 28, 2016

leebyron Oct 28, 2016

langpavel commented Oct 28, 2016

leebyron commented Oct 28, 2016

langpavel commented Oct 28, 2016

langpavel commented Oct 28, 2016 •

edited

Loading

langpavel commented Nov 1, 2016 •

edited

Loading

leebyron commented Nov 1, 2016

langpavel commented Nov 1, 2016 •

edited

Loading

leebyron commented Nov 1, 2016 •

edited

Loading

langpavel commented Nov 1, 2016

langpavel commented Nov 1, 2016

leebyron commented Nov 1, 2016

nodkz commented Nov 1, 2016

langpavel commented Nov 1, 2016

		obj[fieldName] = fieldValue;
		} else if (fieldValue !== null) {

language support for NullValue #544

language support for NullValue #544

Conversation

langpavel commented Oct 27, 2016 • edited Loading

langpavel commented Oct 27, 2016 • edited Loading

leebyron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

langpavel Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

langpavel commented Oct 28, 2016

leebyron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

langpavel commented Oct 28, 2016

leebyron commented Oct 28, 2016

langpavel commented Oct 28, 2016

langpavel commented Oct 28, 2016 • edited Loading

langpavel commented Nov 1, 2016 • edited Loading

leebyron commented Nov 1, 2016

langpavel commented Nov 1, 2016 • edited Loading

leebyron commented Nov 1, 2016 • edited Loading

langpavel commented Nov 1, 2016

langpavel commented Nov 1, 2016

leebyron commented Nov 1, 2016

nodkz commented Nov 1, 2016

langpavel commented Nov 1, 2016

langpavel commented Oct 27, 2016 •

edited

Loading

langpavel commented Oct 27, 2016 •

edited

Loading

langpavel Oct 28, 2016 •

edited

Loading

langpavel commented Oct 28, 2016 •

edited

Loading

langpavel commented Nov 1, 2016 •

edited

Loading

langpavel commented Nov 1, 2016 •

edited

Loading

leebyron commented Nov 1, 2016 •

edited

Loading