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

Avoid wrapping args with partial when avoiding optionals #9673

Merged

Conversation

maclockard
Copy link
Contributor

@maclockard maclockard commented Sep 14, 2023

Description

Makes it so arguments don't get wrapped with a Partial when configured to avoid optionals for input values. Without this change, avoidOptionals = true ends up being inconsistent since it will still use optional input values in the case there is only nullable arguments.

Related #9438

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Added a test that avoid optionals does not wrap the arguments with a partial when configured.

Test Environment:

  • OS: macOS
  • @graphql-codegen/typescript-resolvers: 4.0.1
  • NodeJS: 16.x

Checklist:

  • I have followed the CONTRIBUTING doc and the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Further comments

I think this should be fairly uncontroversial since it is preserving the intention of the avoid optionals config.

@changeset-bot
Copy link

changeset-bot bot commented Sep 14, 2023

🦋 Changeset detected

Latest commit: efb17e2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@graphql-codegen/visitor-plugin-common Patch
@graphql-codegen/typescript-resolvers Patch
@graphql-codegen/typescript-document-nodes Patch
@graphql-codegen/gql-tag-operations Patch
@graphql-codegen/typescript-operations Patch
@graphql-codegen/typed-document-node Patch
@graphql-codegen/typescript Patch
@graphql-codegen/graphql-modules-preset Patch
@graphql-codegen/client-preset Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@maclockard
Copy link
Contributor Author

Let me know whether a minor or patch bump is preferred for the relevant packages

@saihaj
Copy link
Collaborator

saihaj commented Oct 19, 2023

Let me know whether a minor or patch bump is preferred for the relevant packages

Hey @maclockard thanks for creating this fix. Can you please run yarn changeset and add a patch changeset explaining your change so we can get this merged. Learn more about changeset

@saihaj saihaj added the waiting-for-answer Waiting for answer from author label Oct 19, 2023
@maclockard
Copy link
Contributor Author

@saihaj Added a changeset!

@maclockard
Copy link
Contributor Author

@saihaj anything else needed to merge this?

@saihaj saihaj removed the waiting-for-answer Waiting for answer from author label Jan 17, 2024
@saihaj
Copy link
Collaborator

saihaj commented Jan 17, 2024

thank you @maclockard

@saihaj saihaj merged commit 7718a81 into dotansimha:master Jan 17, 2024
11 of 13 checks passed
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.

2 participants