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

fix: Fix input object field introspection with missing defaultValue properties #101

Merged
merged 5 commits into from
Mar 3, 2024

Conversation

llllvvuu
Copy link
Contributor

@llllvvuu llllvvuu commented Mar 2, 2024

Summary

If I have no defaultValue, getInputObjectTypeRec will infer my field as foo?: string | null when it should be foo: string. This is because getInputObjectTypeRec only checks for explicity undefined | null. Add a test case and fix for this.

Set of changes

  • Additional "description" field in test fixture, which has no defaultValue
  • Bugfix and test

Copy link

changeset-bot bot commented Mar 2, 2024

🦋 Changeset detected

Latest commit: bf1a7d1

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

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

@kitten kitten changed the title fix(getInputObjectTypeRec): handle no default value fix: Fix input object field introspection with missing defaultValue properties Mar 2, 2024
Copy link
Member

@kitten kitten left a comment

Choose a reason for hiding this comment

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

Nice catch!

I've got a suggestion for the changeset (just a note, this is the exact text that goes in the changelog, so it needs to be user-facing). Same suggestion is already applied to the PR title (fix(x) is generally reserve for sub-packages)

I've got a larger suggestion here for the type change. It seems a bit long and wordy, and as far as I can tell, is a complex multi-part match for a change we can probably do by just adding ? to defaultValue.
Since it's typed as defaultValue?: T | null, and T | undefined | null isn't equivalent, this minimal change should do the trick and seems to pass tests for me locally.

.changeset/strange-nails-ring.md Outdated Show resolved Hide resolved
src/variables.ts Outdated Show resolved Hide resolved
llllvvuu and others added 2 commits March 2, 2024 17:17
Co-authored-by: Phil Pluckthun <phil@kitten.sh>
Co-authored-by: Phil Pluckthun <phil@kitten.sh>
@llllvvuu
Copy link
Contributor Author

llllvvuu commented Mar 3, 2024

Thanks for the suggestions! I didn't know about ? and it works for me.

@kitten kitten merged commit 4c817bc into 0no-co:main Mar 3, 2024
1 check passed
@github-actions github-actions bot mentioned this pull request Mar 2, 2024
# 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