RFC: Client Controlled Nullability Operators Implementation

[twof]

Implementation of https://github.com/graphql/graphql-spec/issues/867 along with the suggested ? syntax.

Here's a doc with a list of some of the important test cases and the justifications for their behavior.

This PR includes work done by @xuewei8910, @aprilrd, @magicmark, and @lizjakubowski.

[linux-foundation-easycla[bot]]

The committers are authorized under a signed CLA.

• :white_check_mark: Alex Reilly (650f4037bf754019d5a0e424ab73536cbcca33bf, 84d6b646faeaeade63e5895a853a42066ae6a004, 9b8f6f3375522cfd2e0c216e858ef41f9a239a7f, f287b635702db3a006e4cc27f135250bb4142ed2, 8707555f0ccaa9759b4dc70d251588737b2cd6d1, c6c196dc86f15461e88077d0ba4a1966afd25cfc, 306c73033509afa5471ff1ac71c2d044db8bc69f, 7398dcfe178959664e41e84be5fa00a6f5ce5068, bb91eac3a139986c51a836de615ed40239e87fc0)

[twof]

The committers are authorized under a signed CLA.

• :white_check_mark: Alex Reilly (650f4037bf754019d5a0e424ab73536cbcca33bf, 84d6b646faeaeade63e5895a853a42066ae6a004, 9b8f6f3375522cfd2e0c216e858ef41f9a239a7f, f287b635702db3a006e4cc27f135250bb4142ed2, 8707555f0ccaa9759b4dc70d251588737b2cd6d1, c6c196dc86f15461e88077d0ba4a1966afd25cfc, 306c73033509afa5471ff1ac71c2d044db8bc69f, 7398dcfe178959664e41e84be5fa00a6f5ce5068, bb91eac3a139986c51a836de615ed40239e87fc0, 1769df88d92aa5988a35d5afc0b222b7c93d1fc0)

[saihaj]

@twof try rebasing and overwriting author on commits https://stackoverflow.com/a/54363151/11321732

[twof]

@saihaj Thanks! That worked.

[IvanGoncharov]

@twof I enabled CI, and some tests are failing.

[twof]

@IvanGoncharov Thanks! I'll go through these issues.

[twof]

@IvanGoncharov Mind kicking this off again? I think I've resolved the issues. I also ran the test coverage checker locally, and it looks like we ought to be good to go I think.

[twof]

Thanks! I'll fix that spelling

[twof]

Hello again! I've implemented the list syntax described here: https://github.com/graphql/graphql-spec/pull/895#issuecomment-963909931

I'd like a code review on this before I add the list syntax to the spec, in case the implementation ends up being dramatically different after I apply any feedback I get. It's a large PR, so if there's any way I can make reviewing this easier, I'd be happy to do so.

[twof]

Extracted changes that don't need to be in this PR. More to come: https://github.com/graphql/graphql-js/pull/3376 https://github.com/graphql/graphql-js/pull/3377

[IvanGoncharov]

Let me know if there are any notes I can write up that might help you and others review this PR. I fully understand that it's quite large!

@twof Great idea. Let's do first round of review where I check "architectural stuff", e.g. AST design. This round can provoke quite big changes so I suggest writing notes after it finishes.

[twof]

@IvanGoncharov Sounds like a plan. Thanks!

[twof]

@IvanGoncharov I'll take a look at those failing jobs in the morning, but npm test is passing on my machine. In the meantime if you'd like to look at the changes I've made and let me know what you think, the architecture is ready for review again.

I'll go over everything and do some cleanup in the morning, so don't worry too much about the details for now. Also let me know if there are any portions you'd like me to write up explanations for.

Summary of changes:

• ComplexRequiredStatus has been broken up into three Node interfaces.
• Printing and parsing is now much simpler. You were totally right about that.
• modifiedOutputType now also uses a visitor.
• Based on feedback on the spec PR, I'm requiring that nullability modifiers match the list depth for the field they're modifying. If I get pushback on that, we can loosen that restriction, or we can loosen it in a later release without making any breaking changes.

Todo:

• People haven't landed on behavior for the question mark syntax yet, but that's liable to cause significant changes to this implementation.

[twof]

@IvanGoncharov Hey Ivan. During the meeting last Thursday you mentioned that you'd be open to releasing this feature behind an experimental flag while it's still in review, and you had some other features that did something similar in the past. Are you able to point me towards an example of a feature that used an experimental flag? I can implement something similar as part of this PR if you're still interested in doing that.

[saihaj]

Hey @twof we currently publish defer and stream as experimental flag you can read more here: https://github.com/graphql/graphql-js#experimental-features. I think for this to happen we first need to get your this branch to the repo. So @IvanGoncharov can help you give rights and set up a branch here which you can push to. Then each time there are changes we just publish a new experimental tag example: https://github.com/graphql/graphql-js/releases/tag/v16.0.0-experimental-stream-defer.5.

But I think this approach really isn't working well and @IvanGoncharov wanted to merge everything to main so we can have unstable main and have canary releases. But as per JS-WG it was suppose to happen last Monday or Wednesday (as per a comment I saw) but no updates on that end. But hopefully we can get something this week and have this merged soon cause I want to play around with this feature :) cause I don't want to build myself or start publishing on my fork.

[saihaj]

but npm test is passing on my machine

I assume you are on node v16 cause v16 tests are the only ones passing.

[twof]

@saihaj Great! Let me know if there's anything I can do to help with the experimental flag setup, or if yall have anything else I can fix up about the PR. It looks like most checks are passing.

[IvanGoncharov]

@twof I suggest moving in steps. As the first step let's merge support for ! and ? in AST (parse, print, AST types, visit, etc.). Please split out a smaller PR with just AST changes similar to this one https://github.com/graphql/graphql-js/pull/1141 (note it's more than 4 years old, so code changed a bit since then). I think one experimental flag for both ! and ? would be enough. Also please make a short summary of the discussion around ! and ? syntax (only syntax) and open questions if any and add them in the commit message (or PR message).

That way we will merge noncontroversial stuff first and focus on stuff that needs disscussion. Also, it will unblock your work with apollo-ios and graphql-codegen.

[twof]

@IvanGoncharov Here's the PR with the stuff you asked for: https://github.com/graphql/graphql-js/pull/3418

[twof]

Hey @michaelstaib! Thanks for the review. I'm in the process of breaking up this PR into smaller changesets. I've addressed your feedback for the ? and ! portions of the proposal in https://github.com/graphql/graphql-js/pull/3418 and your feedback related to the list syntax in https://github.com/twof/graphql-js/pull/2.

@IvanGoncharov @saihaj In order to reduce confusion, would it be helpful to close this PR and designate https://github.com/graphql/graphql-js/pull/3418 as the canonical implementation branch for this RFC?