-
Notifications
You must be signed in to change notification settings - Fork 411
feat(types): Type check Field defaultValue #2330
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
Conversation
Thanks for your contribution @martinlehoux! Would you like to update tests as well (possibly leveraging the |
@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 |
@kamilmysliwiec I added a test file for the |
@kamilmysliwiec Hi, is there anything I can do to push this forward? |
@kamilmysliwiec Is it still something you'd like to see added to the codebase? |
There was a problem hiding this 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be Inner[]
?
There was a problem hiding this comment.
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 ^^
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 |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently, when using
@Field(() => MyType, { defaultValue: ... })
, thedefaultValue
type is not checked asMyType
at compile time. It can lead to runtime errors if you refactorMyType
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: ... })
, thedefaultValue
type is checked and there is a Typescript error if it's notMyType
orMyType[]
respectively.The feature is not available when using
@Field({ defaultValue: ... })
, it defaults to the current behavior of allowingany
value.Does this PR introduce a breaking change?
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.