Semantic nullability document directive

[twof]

No description provided.

[twof]

These are just tests so it's not the biggest deal, but is there a better way to retrieve the node for use in error construction? This is workable, but unwieldy.

[yaacovCR]

Because we do not yet enable noUncheckedIndexAccess (see #3867) I think you could just index it via 0 at this point although I think what you are doing is preferred, for example

graphql-js/src/execution/subscribe.ts

Line 217 in 423e72d

const [responseName, fieldNodes] = [...rootFields.entries()][0];

Has been changed in main to your method.

You could assert that you have an executable definition or selection node instead of using typescript casting….

In fact, I think that needs to be improved in main for the example I gave!

graphql-js/src/execution/execute.ts

Line 2241 in 831c121

const firstRootField = groupedFieldSet.entries().next().value as [

[yaacovCR]

graphql-js/src/language/predicates.ts

Line 24 in 423e72d

export function isExecutableDefinitionNode(

[yaacovCR]

Paid greater attention when working on twof#5 and I think the answer is that generally the tests use expectJSON and some other testing deduplication, added second commit there demonstrating use.

[twof]

Notes for reviewers

I'm looking for review on everything but utilities. I'm going to let @benjie handle those. Once I've been given a go ahead on the implementation, I'll write the spec edits.

There are a large number of files failing linting. Let me know if there's anything I need to do to fix that.

Coverage is passing on my machine. Unclear why it's not passing CI. It also seems to be treating one of my new test files as uncovered code.

[yaacovCR]

Is there a reason for the introduction of two new wrapping types as opposed to a single one? Is it because of an unresolved syntax question or is it expected to remain in the final implementation?

I could see this being useful if we were to actually allow multiple coexisting syntax variants as dictated by arguments on the semantic nullability directive (but I am not in favor of that additional complexity).

[twof]

Is there a reason for the introduction of two new wrapping types as opposed to a single one? Is it because of an unresolved syntax question or is it expected to remain in the final implementation?

I could see this being useful if we were to actually allow multiple coexisting syntax variants as dictated by arguments on the semantic nullability directive (but I am not in favor of that additional complexity).

In the first commit with the parsing implementation I had a single type.

The printer was the main driver here. In the parser there was a clear place to set the useSemanticNullability mode flag, but with the printer it wasn't so obvious.

[BoD]

Suggested change

	* - SemanticNonNullType
	* - SemanticNonNullType
	* - SemanticNullableType

[yaacovCR]

The printer was the main driver here. In the parser there was a clear place to set the useSemanticNullability mode flag, but with the printer it wasn't so obvious.

I raised twof#5 as a proof-of-concept for what it might look like with a single new wrapper type.

I stored within the created GraphQLSchema whether it was created usingSemanticNullability and built printing and execution related changed off of that flag.

You can add in semantically nullable types even in a schema that is not using usingSemanticNullability and they will be ignored. In particular, to remain with simply one version of the Introspection types, I modified the built-in Introspection types where I thought to be appropriate to use the new GraphQLSemanticNullable wrapping types; these types are unwrapped but otherwise ignored even when usingSemanticNullability is not set.