Skip to content
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

Better wording for GraphQL's doc section #3149

Closed
1 of 5 tasks
kasir-barati opened this issue Nov 21, 2024 · 3 comments · Fixed by #3150
Closed
1 of 5 tasks

Better wording for GraphQL's doc section #3149

kasir-barati opened this issue Nov 21, 2024 · 3 comments · Fixed by #3150

Comments

@kasir-barati
Copy link
Contributor

I'm submitting a doc improvement request

  • Regression
  • Bug report
  • Feature request
  • Documentation issue or request (new chapter/page)
  • Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

So the current doc is a bit misleading and causes confusion. I also learned that we might want to add some "heads up" notes about nullable fields as we have gone over it here: https://discord.com/channels/520622812742811698/1309067345943199774/1309067345943199774

Expected behavior

Instead of

nullable: for specifying whether a field is nullable (in SDL, each field is non-nullable by default);

IMO it is more concise to say: nullable: for specifying whether a field is nullable (in @nestjs/graphql, each field is non-nullable by default);

Minimal reproduction of the problem with instructions

n/a.

What is the motivation / use case for changing the behavior?

Less confusion after reading the doc

Environment

n/a.

For Tooling issues:

n/a.

Others:

One more thing that is also something I learned in that discord discussion and was not mentioned in the doc was how you should type your object types:

// wrong
@Field(() => String, {nullable: true}) 
id?: string;
// correct
@Field(() => String, {nullable: true}) 
id?: string | null;

And here is a relevant PR that lead to this issue: #3147

@kamilmysliwiec
Copy link
Member

Would you like to create a PR for this issue?

@kasir-barati
Copy link
Contributor Author

Yep, I love to do so. I am gonna open that PR right away, then we can talk about specifics there since I am not sure changes that I make would be appropriate.

@kamilmysliwiec
Copy link
Member

Let's track this here #3150

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 a pull request may close this issue.

2 participants