Skip to content

Propose single wrapping type #4339

New issue

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

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

Already on GitHub? # to your account

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Feb 2, 2025

This reduces the new AST-nodes to only be for the newly introduced type, this does make it so that when we invoke print we have to rely on the user to either specify that we're in semantic nullability mode or we could do a pre-traverse and when we enter a node with semantic-non-null we toggle it on ourselves.

The main reasoning behind removing the new name for our existing null type is that I would prefer to be backwards compatible in terms of schema structure. This because it might become complex for people to reason about composed schemas, i.e. a lot of individually parsed schemas that later on compose into a larger one.

I know that technically this is covered because in the classic ones we'll have the non wrapped null type and in the modern ones we'll have the semantic nullable wrapped type. For schema-builders like pothos and others I think this is rather complex to reason about and to supply us with. I would instead choose to absorb this complexity in the feature and stay backwards compatible.

This also sets us up for the SDL not being a breaking change, we only add one AST-type, what's left now is to settle on a semantic non-null syntax and making everything backwards compatible.

@JoviDeCroock JoviDeCroock requested a review from a team as a code owner February 2, 2025 14:58
Copy link

github-actions bot commented Feb 2, 2025

Hi @JoviDeCroock, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

Copy link
Contributor

@twof twof left a comment

Choose a reason for hiding this comment

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

I think this works, and I definitely prefer this over having two types. Go ahead an merge it into your other branch, and I'll give you a more thorough review of that one tomorrow.

@JoviDeCroock
Copy link
Member Author

@twof my main "desire" would be to only introduce new syntax for the new type as well but I feel that is a more sensitive matter at this moment in time so I'll leave it at that and merge the descriptions as well

@JoviDeCroock JoviDeCroock merged commit 869ca46 into semantic-nullability-rfc-implementation Feb 5, 2025
28 of 32 checks passed
@JoviDeCroock JoviDeCroock deleted the one-wrapping-type-proposal branch April 24, 2025 13:18
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants