Addon-knobs: improve types via generics and readonlyarray #7411
Conversation
emilio-martinez
added
bug
addon: knobs
maintenance
emilio-martinez
requested review from
alexandrebodin,
alterx,
leonrodenburg and
theinterned
as code owners
|
This pull request is automatically deployed with Now. Latest deployment for this branch: https://monorepo-git-fork-emilio-martinez-fix-addon-knobs-d58aac.storybook.now.sh |
emilio-martinez
force-pushed
the
fix/addon-knobs-generics-and-readonlyarray
branch
from
4358902 to
0587c68
Compare
|
@shilman any chance we could get this one in sooner rather than later. It's a blocker to upgrade to a version higher than ~.35 |
emilio-martinez
force-pushed
the
fix/addon-knobs-generics-and-readonlyarray
branch
from
51ae10e to
d3d6548
Compare
emilio-martinez
force-pushed
the
fix/addon-knobs-generics-and-readonlyarray
branch
from
d3d6548 to
98265c7
Compare
|
@emilio-martinez, is this working for you in your project? I'm getting the following errors in mine: ERROR in /node_modules/@storybook/addon-knobs/dist/components/types/Array.d.ts ERROR in /node_modules/@storybook/addon-knobs/dist/components/types/Options.d.ts ERROR in /node_modules/@storybook/addon-knobs/dist/components/types/Options.d.ts ERROR in /node_modules/@storybook/addon-knobs/dist/components/types/Options.d.ts ERROR in /node_modules/@storybook/addon-knobs/dist/components/types/Options.d.ts |
|
@CodeByAlex haven't been getting that, but I'll ping you on Discord |
|
|
||
| type ArrayTypeKnobValue = string[]; | ||
| export type ArrayTypeKnobValue = string[] | readonly string[]; |
There was a problem hiding this comment.
@emilio-martinez Is this breaking type change intentional? Using numbers will cause a typescript error.
There was a problem hiding this comment.
To quote the README for this add-on:
array
Allows you to get an array of strings from the user.
Unfortunately, the package from DefinitelyTyped is inaccurate—that field uses a text-based input of comma delimited values, so even if you were to pass a number array initially, it would subsequently return a string array on change, i.e., here's no number parsing logic for that control.
That said, I'd love to hear more about your use case and help in any way I can. Would you mind sharing?
There was a problem hiding this comment.
Hey @emilio-martinez Thanks for the reply. Yeah it seems like you guys are just having to bite the bullet on these type changes going from javascript to typescript, which I totally feel your pain.
This is my use case 👇. It's fixable by just parsing the values and there is probably a better way to go about it. Just got this error when upgrading from v5.1.9 to v5.2.0-beta.19.
https://github.com/elastic/elastic-charts/blob/8ebe600df1ae476e25032ab1c7707de398143e43/stories/annotations.tsx#L41
I'm curious if you guys are gradually upgrading you codebase to ts or doing it in a single release (I'm too lazy to check for myself)? If it is in a single release why wouldn’t you just release ts in v6 rather than producing all these stricter type errors on a minor release?
Thanks 👍
There was a problem hiding this comment.
TS migration is an epic started at the end of 2018, we recently reached an important step as 50% of Storybook codebase in TS 🎉.
Migration progress is followed in #5030 😉. Avoiding breaking changes is one of our goals but we somethings found weird/wrong types/docs even in SB own codebase.
There was a problem hiding this comment.
@gaetanmaisse Sweet congrats on the big 5-0!! 🎉 All the best keep up the great work.
There was a problem hiding this comment.
Actually, there are many work around and we don't buy in strict type and noImplicitAny at day one (I think is for easier migration). For that reason, there are sheer amount of any flood in our typing. That somehow explains why we inevitably will have breaking changes in type system, we need to improve it gradually due to the fact that the codebase is pretty huge. But I think this brings more organization and community collaboration to make it awesome in long term.
Issue: Needed to add more fidelity to the knobs value types to cover even more test cases, including
ReadonlyArraycases (#7348) and enum cases (#7420).What I did
ReadonlyArraymay be provided (Typescript seems to treat this differently as a normal array)How to test
The knobs addon spans a fair number of use cases; therefore, I had the need to create a set of test cases with which I want to ensure compilation passes. To this effect, I added a file under
src/__types__/with a series of test cases. The successful compilation of this file should indicate types are passing correctly, at least as far as that file is intending to document.I don't believe there's currently an established process for testing type definitions specifically in the repo, so I'd love to get thoughts/feedback on the approach.