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

fix(Tabs): do not infer type of modelValue from defaultValue prop #866

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

DamianGlowala
Copy link
Contributor

@DamianGlowala DamianGlowala commented Apr 21, 2024

This PR:

  • prevents defaultValue from being inferred by the modelValue prop
  • replaces StringOrNumber with locally declared ModelValue type (and, most importantly, replaces it in the docs with string | number - its simplest and easiest form)
  • renames T generic param to TModelValue for the sake of clarity
  • bumps TypeScript version so that NoInfer util can be used

(Other errors seem to be revealed in CI which need to be addressed.)

@DamianGlowala DamianGlowala changed the title fix(Tabs): do not infer model value from defaultValue prop fix(Tabs): do not infer type of modelValue from defaultValue prop Apr 22, 2024
@zernonia
Copy link
Collaborator

Thanks for the PR @DamianGlowala . Seems like the Typescript bump causing the error in CI. Is it neccessary to have NoInfer here?

@DamianGlowala
Copy link
Contributor Author

DamianGlowala commented Apr 23, 2024

NoInfer prevents a bug when you use a modelValue combined with defaultValue. Currently, generic param will merge these two, which means that modelValue isn't the only source of truth and can be extended, which shouldn't be the case.

As far as I remember, this utility type has been introduced in TS v5.4, hence a need for a bump.

Copy link
Collaborator

@zernonia zernonia left a 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

@DamianGlowala
Copy link
Contributor Author

@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 @ts-expect-error, shall we proceed with the merge? 😄

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.

None yet

2 participants