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

fix(graphql): generate fields as optional when they contain arguments #2623

Merged
merged 1 commit into from
Feb 24, 2023
Merged

fix(graphql): generate fields as optional when they contain arguments #2623

merged 1 commit into from
Feb 24, 2023

Conversation

arthurtemple
Copy link
Contributor

@arthurtemple arthurtemple commented Feb 2, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Fields with arguments are usually resolved using a separate method with the @ResolveField() annotation.
Having such field generated as a required property thus proves quite cumbersome as it forces setting a value for this field in the result of parent @Query() method.

What is the new behavior?

Fields with arguments are always generated as optional.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Using /dev/null as argument to explore looks unfortunate to me, please advise for a better solution.

Fields with arguments are usually resolved using a separate method with the
@ResolveField() annotation.
Having such field generated as a required property thus proves quite cumbersome
 as it forces assigning a value in the result of parent @query() method.

This makes fields with arguments always optional.
Copy link
Member

@jmcdo29 jmcdo29 left a comment

Choose a reason for hiding this comment

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

LGTM

@kamilmysliwiec
Copy link
Member

This introduces a (minor but still) breaking change

@arthurtemple
Copy link
Contributor Author

This introduces a (minor but still) breaking change

Thanks for the feedback!
Apart from the breaking change, is there anything to adjust in this PR?

@kamilmysliwiec kamilmysliwiec changed the base branch from master to next February 24, 2023 11:56
@kamilmysliwiec kamilmysliwiec merged commit 15fd4e3 into nestjs:next Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants