Skip to content

CCN experiment with execution #3510

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 35 commits into from
Closed

Conversation

twof
Copy link
Contributor

@twof twof commented Mar 12, 2022

No description provided.

@netlify
Copy link

netlify bot commented Mar 12, 2022

Deploy Preview for compassionate-pike-271cb3 failed.

Name Link
🔨 Latest commit bf64b72
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/629fc2bab774750009d92ba8

Comment on lines 619 to 697
/*
options:
- pass in field nodes, so we can see which are required
- create a new GraphQL output type to represent required and optional fields

? on its own marks a field nullable
All ! are caught by the next ?, no matter the level
! + ! = !
! + ? = 0
! + ! + ? = 0

These are executed top down which means we can't follow a
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete

@@ -662,6 +709,7 @@ function completeValue(
info,
path,
result,
false,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undo probably

@@ -714,6 +762,7 @@ function completeListValue(
info: GraphQLResolveInfo,
path: Path,
result: unknown,
isRequiredChain: Boolean,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undo

// Note: Client Controlled Nullability is experimental
// and may be changed or removed in the future.
readonly required?: ListNullabilityNode | NullabilityDesignatorNode;
readonly isInRequiredChain: Boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will likely become a new required status instead since it feels like it's another case that isn't nullable or non nullable, but impacts null propagation rules all the same.

Comment on lines 467 to 472
// const isRequiredChain: Boolean = ((selectionSet?.selections) ?? [])
// .reduce((accumulator: Boolean, element: SelectionNode) => {
// if (accumulator) {
// return true
// } else if ()
// })
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleanup

Comment on lines 483 to 490
for (const selection of selectionSet?.selections ?? []) {
const field = selection as FieldNode;
if (selection.kind == Kind.FIELD) {
if (field.isInRequiredChain) {
isRequiredChain = true;
break;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see if TS has some contains(where:) method that we can replace this with

@github-actions
Copy link

The latest changes of this PR are available on NPM as
graphql@16.3.0-canary.pr.3510.942fbd8ea3d803e74908fabecbe03dfaefe3e5c8
Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR:
npm install --save graphql@canary-pr-3510

@github-actions
Copy link

The latest changes of this PR are available on NPM as
graphql@16.3.0-canary.pr.3510.5099f4491dc2a35a3e4a0270a55e2a228c15f13b
Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR:
npm install --save graphql@canary-pr-3510

Comment on lines +663 to +697
options:
- pass in field nodes, so we can see which are required
- create a new GraphQL output type to represent required and optional fields

? on its own marks a field nullable
All ! are caught by the next ?, no matter the level
! + ! = !
! + ? = 0
! + ! + ? = 0

if propagation path is undefined, that could mean that there are no required fields
or that could mean that there are no optional fields. Path can't represent an empty path
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleanup

Comment on lines 31 to 52

while (isListType(getNullableType(typeStack[typeStack.length - 1]))) {
const list = assertListType(
getNullableType(typeStack[typeStack.length - 1]),
);
const elementType = list.ofType as GraphQLOutputType;
typeStack.push(elementType);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only populate typeStack after we've confirmed that nullabilityNode exists to improve performance

@@ -104,4 +104,4 @@ export type { BreakingChange, DangerousChange } from './findBreakingChanges';
// Wrapper type that contains DocumentNode and types that can be deduced from it.
export type { TypedQueryDocumentNode } from './typedQueryDocumentNode';

export { modifiedOutputType } from './applyRequiredStatus';
export { applyRequiredStatus as modifiedOutputType } from './applyRequiredStatus';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why I exported this under a different name. Probably revert.

Comment on lines +20 to +27
/**
* List element nullability designators need to use a depth that is the same as or less than the
* type of the field it's applied to.
*
* Otherwise the GraphQL document is invalid.
*
* See https://spec.graphql.org/draft/#sec-Field-Selections
*/
Copy link
Contributor Author

@twof twof May 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validation step isn't accurate. Update to include loosened rules.

};

const modified = visit(nullabilityNode, applyStatusReducer);
// modifiers must be exactly the same depth as the field type
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Designators

@twof twof force-pushed the CCNExperimentWithExecution branch from a3bef80 to 40df6bc Compare June 1, 2022 00:12
@calvincestari
Copy link

Hi @twof, are you still championing this work through the GraphQL WG? I'm open to chat about it and contribute to getting this moving forward again.

@twof
Copy link
Contributor Author

twof commented Jul 4, 2023

Hi @twof, are you still championing this work through the GraphQL WG? I'm open to chat about it and contribute to getting this moving forward again.

Hi @calvincestari! Took me a bit to see this sorry about that. If you're still interested, can you message me in the #client-controlled-nullability channel on the graphql discord? I passed the project on to @fotoetienne but I'm sure he'd appreciate the help.

@calvincestari
Copy link

Hi @twof, are you still championing this work through the GraphQL WG? I'm open to chat about it and contribute to getting this moving forward again.

Hi @calvincestari! Took me a bit to see this sorry about that. If you're still interested, can you message me in the #client-controlled-nullability channel on the graphql discord? I passed the project on to @fotoetienne but I'm sure he'd appreciate the help.

Will do. I'm away this week but I'll follow up next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants