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

feat(types): Type check Field defaultValue #2330

Merged
merged 1 commit into from Feb 24, 2023
Merged

feat(types): Type check Field defaultValue #2330

merged 1 commit into from Feb 24, 2023

Conversation

martinlehoux
Copy link

@martinlehoux martinlehoux commented Aug 12, 2022

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: Typing improvement

What is the current behavior?

Currently, when using @Field(() => MyType, { defaultValue: ... }), the defaultValue type is not checked as MyType at compile time. It can lead to runtime errors if you refactor MyType but forget to update the default value.

What is the new behavior?

When using the @Field(() => MyType, { defaultValue: ... }) or the array one @Field(() => [MyType], { defaultValue: ... }), the defaultValue type is checked and there is a Typescript error if it's not MyType or MyType[] respectively.

The feature is not available when using @Field({ defaultValue: ... }), it defaults to the current behavior of allowing any value.

Does this PR introduce a breaking change?

  • Yes
  • No

I think it doesn't, given there is an any fallback, but I may need to test more thoroughly (I don't know how to add type tests in the test suite).

Other information

The same behavior could be implemented for others decorators to some extend.

@kamilmysliwiec
Copy link
Member

Thanks for your contribution @martinlehoux! Would you like to update tests as well (possibly leveraging the // @ts-expect-error helper https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-9.html#-ts-expect-error-comments)?

@martinlehoux
Copy link
Author

@kamilmysliwiec Thanks for the link, I didn't know it existed! As for the tests, could you point me to where I should add it. I have a hard time finding the tests related to the @field decorator

@martinlehoux
Copy link
Author

@kamilmysliwiec I added a test file for the field.decorator.ts file, please tell me if the format of the test is OK

@martinlehoux
Copy link
Author

@kamilmysliwiec Hi, is there anything I can do to push this forward?

@martinlehoux
Copy link
Author

@kamilmysliwiec Is it still something you'd like to see added to the codebase?

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.

Overall, I think the type looks good. This shouldn't cause any issues with older code bases, yeah? As any is the default used here


class CorrectArray {
@Field(() => [Inner], { defaultValue: [{ test: 'test' }] })
inner: Inner;
Copy link
Member

Choose a reason for hiding this comment

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

Should this be Inner[]?

Copy link
Author

Choose a reason for hiding this comment

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

I guess you're right ^^

@kamilmysliwiec
Copy link
Member

PR looks great! However, we can't merge it in yet as it introduces a (minor but still) breaking change (even though in theory it shouldn't break when defaultValue was used as expected)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants