-
-
Notifications
You must be signed in to change notification settings - Fork 148
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(Tabs): do not infer type of modelValue
from defaultValue
prop
#866
base: main
Are you sure you want to change the base?
fix(Tabs): do not infer type of modelValue
from defaultValue
prop
#866
Conversation
defaultValue
propmodelValue
from defaultValue
prop
Thanks for the PR @DamianGlowala . Seems like the Typescript bump causing the error in CI. Is it neccessary to have |
As far as I remember, this utility type has been introduced in TS v5.4, hence a need for a bump. |
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.
@DamianGlowala can you check why the pipeline is failing? Perhaps there's some changes in 5.4 that causes some infinite loop
@zernonia I have resolved 3 out of 4 issues (improved/restructured generics a bit!). If you're fine with the remaining issue being silenced with |
@@ -45,6 +45,5 @@ type ScrollBodyOption = { | |||
|
|||
type AcceptableValue = string | number | boolean | Record<string, any> | |||
type ArrayOrWrapped<T> = T extends any[] ? T : Array<T> | |||
type StringOrNumber = string | number |
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 think having this type named as StringOrNumber
is much better than ModelValue
. ModelValue
naming can be ambiguous and if more component using this name it's gonna cause some confusion.
Let's stick it back to StringOrNumber
please 😁
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.
Why would it be confusing to name something more closely resembling its purpose, especially knowing the ModelValue
type is placed inside of the component it is related to (unlike generic StringOrNumber
)? Two reasons I dislike this abstraction:
- reduces cognitive load when mentioned in the docs (as a user, I'd much rather see a prop is of type
string | number
thanStringOrNumber
) - why should we abstract something so short and obvious? If the type ever changes, so does the name
StringOrNumber
. If the component's model value ever changes, it will still remain a model value. I can't see the purpose of having it.
Nevertheless, let me know if the above makes sense at all - I'm happy to pick whichever option we decide on 😄
I see |
@zernonia Yes, indeed! 😄It's only in one place. Other three errors were to do with destructuring variables which TypeScript didn't have enough 'confidence' they were objects. I slightly adjusted the generics and now it works great. |
This PR:
defaultValue
from being inferred by themodelValue
propStringOrNumber
with locally declaredModelValue
type (and, most importantly, replaces it in the docs withstring | number
- its simplest and easiest form)T
generic param toTModelValue
for the sake of clarityNoInfer
util can be used(Other errors seem to be revealed in CI which need to be addressed.)