Added support for null input arguments (Improved) #374
Conversation
|
Thanks for the PR! You can fix the build by temporarily adding the myget.org nuget source to use your changes and make sure all the tests pass. https://www.myget.org/feed/graphql-dotnet/package/nuget/GraphQL-Parser https://www.myget.org/F/graphql-dotnet/api/v3/index.json https://github.com/graphql-dotnet/graphql-dotnet/blob/master/src/NuGet.config |
|
I think your CI is broken? The myget feed hasn't updated in a few days. Appveyor has the package but the feed asks me for a password so I guess that isn't public. If I download it and use it in a local feed then all the tests pass. |
|
Ah, yeah - its not broken, but the myget deploy is restricted by branch which I forgot about. You could perhaps add your branch to the |
| public class NullValue : AbstractNode, IValue | ||
| { | ||
|
|
||
| public string Value { get; } |
There was a problem hiding this comment.
I think we don't need this Value property at all?
|
Some other places we probably want to update:
|
|
I have resolved the first two comments. I don't think the schema printer needs to know about the NullValue AST object as it can never end up with one. The printer will only deal with actual types or types wrapped in non null never actual values (Ignoring the default which might be null but that's the value not the AST type). The JS project just passes null around being untyped which I did consider doing here but it does make the code a bit more messier in C# land as you end up changing int to int? etc and then having to check everywhere where you use that value that you have a value. |
src/GraphQL/Utilities/AstPrinter.cs
Outdated
|
|
||
| Config<NullValue>(c => | ||
| { | ||
| c.Print(f => null); |
There was a problem hiding this comment.
This should print the text null vs. just a null value.
c.Print(f => "null");
There was a problem hiding this comment.
Are you sure? If you do that you end up with null in a string which correctly fails a couple of unit tests:
Message: Assert.Equal() Failure
↓ (pos 29307)
Expected: ··· "defaultValue": null\r\n }\r\n ]\r\n },\r\n ···
Actual: ··· "defaultValue": "null"\r\n }\r\n ]\r\n },\r···
↑ (pos 29307)
There was a problem hiding this comment.
Yes, that matches the JS project behavior.
https://github.com/graphql/graphql-js/blob/master/src/language/printer.js#L79
There was a problem hiding this comment.
Let me know if you need it squashing once your happy
|
It looks like the JS project uses |
|
A bit pushed for time at the moment so I'll hold off on that one. I tried changing the parser ci branch config but after looking into it they have disabled deployment for pull requests for non trusted people. |
|
I have renamed null -> Nullvalue as reviewed on the parser repo and squashed all the commits. Let me know if there is anything blocking this from being merged. |
|
I have pointed to the custom MyGet feed now the Parser change has been merged. Let me know if you want that commit removing but I think it might be best left in? Edit: I have removed the CI change in response to the below. Here was the green build: https://ci.appveyor.com/project/graphql-dotnet-ci/graphql-dotnet/build/0.17.2.756-jtrsgfnc |
|
It will need to be removed once an official release of v3 of the parser is on Nuget.org. I want to make a few tweaks to the parser project before officially releasing it. I'll do that today. |
@joemcbride did you make any progress on that? |
|
@tlil yes, see th dev branch in that project. |
|
What is the status of this? Wouldn't this reduce the need for multiple GraphQL queries for arguments. Can the feature be applied to Variables as well? Thanks for all the work on the project. I hope to learn a bit more and contribute. |
|
Its been ready for merging for 6 weeks now afaik - I think @joemcbride is waiting for some other unrelated features to do a major version bump. I don't think this feature requires a major version bump itself. Variables look to be working correctly with my PR. |
|
The issue is the building of the Parser nuget package. That build process had to be changed and I haven't been able to 100% verify its working yet. Since this is a behavior change in the parser, yes it requires a major version bump. Ditto for this project. |
|
Got the parser project updated and added #421 which merges these changes. |
| @@ -369,7 +369,7 @@ public void allows_querying_field_args() | |||
| 'kind': 'SCALAR' | |||
| } | |||
| }, | |||
| 'defaultValue': null | |||
| 'defaultValue': 'null' | |||
There was a problem hiding this comment.
This change is making GraphIQL not able to parse the introspection. Is anyone else having this problem?
There was a problem hiding this comment.
Note after reverting this change set, graph iql works as expected.
There was a problem hiding this comment.
@joemcbride should this be sent as a new issue?
There was a problem hiding this comment.
@holm0563 may need to bump the GraphiQL version to one that supports the null keyword perhaps?
There was a problem hiding this comment.
@joemcbride i didn't look at the spec but I'd be shocked if its truly supposed to change to 'null' as a string?
There was a problem hiding this comment.
AH, I see what you're saying.
The JS project treats null different than undefined and NaN. We don't have the "luxury" of 3 different "nulls" in C#. Will have to decide on the best course of action here.
Replaces #306
The other pull looks to be dead and has issues which I have resolved.
Requires graphql-dotnet/parser#17
Build will fail until the other PR is merged, let me know if you need anything else from me.