RFC: Client Controlled Nullability Operators Implementation #3281

@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 · 2021-10-12T05:41:41Z

Thanks! I'll fix that spelling

src/validation/rules/RequiredStatusOnFieldMatchesDefinitionRule.ts

twof · 2021-11-18T07:50:14Z

src/utilities/__tests__/TypeInfo-test.ts

@@ -12,6 +12,7 @@ import { getNamedType, isCompositeType } from '../../type/definition';

 import { buildSchema } from '../buildASTSchema';
 import { TypeInfo, visitWithTypeInfo } from '../TypeInfo';
+import { ComplexRequiredStatus } from '../../language/ast';


I might need a test in this file somewhere

twof · 2021-11-18T07:56:30Z

src/validation/rules/OverlappingFieldsCanBeMergedRule.ts

+    // Errors will have already been handled by RequiredStatusOnFieldMatchesDefinitionRule
+    //  so there's no need to do anything here in the event that modifiedOutputType throws
+    //  an error.
+    try {
+      var modifiedType1 = modifiedOutputType(type1, node1.required);
+      var modifiedType2 = modifiedOutputType(type2, node2.required);
+
+      if (doTypesConflict(modifiedType1, modifiedType2)) {
+        return [
+          [
+            responseName,
+            `they return conflicting types "${inspect(
+              modifiedType1,
+            )}" and "${inspect(modifiedType2)}"`,
+          ],
+          [node1],
+          [node2],
+        ];
+      }
+    } catch {}


Is it okay not handling errors here if I expect the errors these functions throw to be handled in a different rule? Is there any reason to think one rule will be run and the other won't?

twof · 2021-11-18T08:01:07Z

src/validation/rules/RequiredStatusOnFieldMatchesDefinitionRule.ts

+ * A GraphQL document is only valid if all fields selected are defined by the
+ * parent type, or are an allowed meta field such as __typename.
+ *
+ * See https://spec.graphql.org/draft/#sec-Field-Selections


Update this link once I've added this rule to the spec

twof · 2021-11-30T12:29:12Z

@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 · 2021-12-07T03:08:16Z

@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 · 2021-12-07T04:12:11Z

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 · 2021-12-07T04:17:11Z

but npm test is passing on my machine

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

src/utilities/applyRequiredStatus.ts

twof · 2021-12-07T07:00:05Z

@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 · 2021-12-07T11:53:16Z

@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 #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 · 2021-12-08T01:23:46Z

@IvanGoncharov Here's the PR with the stuff you asked for: #3418

src/language/ast.ts

src/language/parser.ts

twof · 2021-12-16T08:52:57Z

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 #3418 and your feedback related to the list syntax in twof#2.

@IvanGoncharov @saihaj In order to reduce confusion, would it be helpful to close this PR and designate #3418 as the canonical implementation branch for this RFC?

…sly calling modifiedOutputType

twof commented Sep 29, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

package-lock.json Outdated Show resolved Hide resolved

twof force-pushed the operatorImplementation branch 2 times, most recently from f44f30d to 05f95f7 Compare October 5, 2021 21:10

twof marked this pull request as ready for review October 5, 2021 21:13

twof mentioned this pull request Oct 7, 2021

[2021-05-13] Query Level Nullability - open PR graphql/graphql-wg#694

Closed

kitten mentioned this pull request Oct 8, 2021

RFC: (graphcache) - Nullability Operators urql-graphql/urql#2018

Closed

IvanGoncharov added the spec RFC Implementation of a proposed change to the GraphQL specification label Oct 8, 2021

IvanGoncharov added the PR: feature 🚀 requires increase of "minor" version number label Oct 8, 2021

twof commented Oct 12, 2021

View reviewed changes

src/execution/execute.ts Outdated Show resolved Hide resolved

twof mentioned this pull request Oct 19, 2021

RFC: Client Controlled Nullability graphql/graphql-spec#895

Closed

twof commented Nov 16, 2021

View reviewed changes

src/validation/rules/RequiredStatusOnFieldMatchesDefinitionRule.ts Outdated Show resolved Hide resolved

twof commented Nov 18, 2021

View reviewed changes

twof added 8 commits November 18, 2021 00:05

apply old changes on top of up to date main branch

0f45eea

tests running

a0c01d3

fixed a couple tests

d17767d

all tests passing

c44ef34

a few additional tests to cover optional cases

0155fff

additional tests

c64a6d8

cleaned up execution tests

44679b4

add exclusive types tests

eb1561b

twof added 7 commits November 30, 2021 01:58

minor cleanup

2472046

fixed some of the overlapping fields tests

c1e1812

required depth validation fixex

cb96767

cleanup leftover references to deleted types

ed2973a

complete test coverage

546bf3e

all tests passing

4c4cbf6

npm test passing

002c999

twof requested a review from IvanGoncharov November 30, 2021 12:20

twof added 2 commits December 2, 2021 01:01

requiring list depth to match type

2e3989c

npm test passes

7715d26

saihaj requested changes Dec 7, 2021

View reviewed changes

src/utilities/applyRequiredStatus.ts Outdated Show resolved Hide resolved

twof added 2 commits December 6, 2021 21:37

remove node16-only api

d490d5b

failing test-case found

6e67e6b

twof requested a review from saihaj December 7, 2021 07:00

twof mentioned this pull request Dec 8, 2021

[RFC] Client Controlled Nullability experiment implementation w/o execution #3418

Merged

michaelstaib reviewed Dec 15, 2021

View reviewed changes

src/language/ast.ts Show resolved Hide resolved

michaelstaib reviewed Dec 15, 2021

View reviewed changes

src/language/ast.ts Show resolved Hide resolved

michaelstaib reviewed Dec 16, 2021

View reviewed changes

src/language/parser.ts Show resolved Hide resolved

twof added 3 commits December 20, 2021 16:54

rework RequiredStatusOnFieldMatchesDefinitionRule so it's not needles…

cc351f4

…sly calling modifiedOutputType

prettier

68701d3

all tests passing and 100% coverage after validation rule update

d15a597

twof closed this Dec 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Client Controlled Nullability Operators Implementation #3281

RFC: Client Controlled Nullability Operators Implementation #3281

twof commented Sep 29, 2021 •

edited

Loading

linux-foundation-easycla bot commented Sep 29, 2021 •

edited

Loading

twof commented Sep 29, 2021 •

edited by linux-foundation-easycla bot

Loading

saihaj commented Sep 30, 2021

twof commented Oct 5, 2021

IvanGoncharov commented Oct 8, 2021

twof commented Oct 12, 2021

twof commented Oct 12, 2021

twof commented Oct 12, 2021

twof Nov 18, 2021

twof Nov 18, 2021

twof Nov 18, 2021

twof commented Nov 30, 2021 •

edited

Loading

twof commented Dec 7, 2021

saihaj commented Dec 7, 2021

saihaj commented Dec 7, 2021

twof commented Dec 7, 2021

IvanGoncharov commented Dec 7, 2021

twof commented Dec 8, 2021

twof commented Dec 16, 2021

RFC: Client Controlled Nullability Operators Implementation #3281

RFC: Client Controlled Nullability Operators Implementation #3281

Conversation

twof commented Sep 29, 2021 • edited Loading

linux-foundation-easycla bot commented Sep 29, 2021 • edited Loading

twof commented Sep 29, 2021 • edited by linux-foundation-easycla bot Loading

saihaj commented Sep 30, 2021

twof commented Oct 5, 2021

IvanGoncharov commented Oct 8, 2021

twof commented Oct 12, 2021

twof commented Oct 12, 2021

twof commented Oct 12, 2021

twof Nov 18, 2021

Choose a reason for hiding this comment

twof Nov 18, 2021

Choose a reason for hiding this comment

twof Nov 18, 2021

Choose a reason for hiding this comment

twof commented Nov 30, 2021 • edited Loading

twof commented Dec 7, 2021

saihaj commented Dec 7, 2021

saihaj commented Dec 7, 2021

twof commented Dec 7, 2021

IvanGoncharov commented Dec 7, 2021

twof commented Dec 8, 2021

twof commented Dec 16, 2021

twof commented Sep 29, 2021 •

edited

Loading

linux-foundation-easycla bot commented Sep 29, 2021 •

edited

Loading

twof commented Sep 29, 2021 •

edited by linux-foundation-easycla bot

Loading

twof commented Nov 30, 2021 •

edited

Loading