Core: Migrate core to TypeScript #12839
Conversation
gaetanmaisse
force-pushed
the
tech/migrate-core-to-TS
branch
from
f950906
to
5f5ac2e
Compare
|
I will push new commits improving typing in the few next days 😉 |
gaetanmaisse
force-pushed
the
tech/migrate-core-to-TS
branch
4 times, most recently
from
26ffe19
to
3e7602a
Compare
…ction Then use this interface in `core` to improve typings
We need to "fake" `@storybook/ui` types to be able to compile properly `@storybook/core`. This is due to a dependency cycle causing `core` to be build at the same time as `ui`: > lerna WARN ECYCLE Dependency cycles detected, you should fix these! > lerna WARN ECYCLE @storybook/react -> @storybook/core -> @storybook/ui -> @storybook/react
…used across the lib Start typing manager presets and config
gaetanmaisse
force-pushed
the
tech/migrate-core-to-TS
branch
from
3e7602a
to
5d81fbd
Compare
gaetanmaisse
force-pushed
the
tech/migrate-core-to-TS
branch
from
5d81fbd
to
f85e888
Compare
|
The types still need some refinement and refactoring but as discussed during the monthly meeting we will merge as it to avoid many conflicts resolution. I added a big warning to avoid exporting these WIP types and I will work to improve them in the next few days/weeks. @yannbf was the typing bug you had in mind yesterday the one I fixed in the last commit? |
Yes, exactly! I was also wondering if options could be |
gaetanmaisse
requested review from
alterx,
igor-dv,
ndelangen,
theinterned and
tmeasday
as code owners
I would say no but I'm really not sure so I didn't take the risk 🙈 Let's see what the experts are saying 😄 |
gaetanmaisse
requested review from
shilman,
yannbf,
tooppaaa and
mrmckeb
and removed request for
theinterned and
igor-dv
| } | ||
|
|
||
| export interface Presets { | ||
| apply( |
There was a problem hiding this comment.
I see the point but isn't too limitative (and potentially incomplete) to list all presets ?
There was a problem hiding this comment.
I somehow need these types to avoid type inference to "return" any on all these calls: https://github.com/storybookjs/storybook/pull/12839/files#diff-d0f0c842d74746ce0b792a604e01fb7ded59c01898321fb1b482ba3ca016ab7dR12-R28
It's not directly the presets but the functions a preset can expose and so for instance webpack function should/must always return a webpack Configuration whatever the preset. Do you agree? 🤔
But it's not 100% clear for me yet, maybe I will get rid of that when improving the typing.
I'm surely missing some parameters that why I introduced a fallback on L45
| import { typeScriptDefaults } from './config/defaults'; | ||
|
|
||
| /** | ||
| * ⚠️ This file contains internal WIP types they MUST NOT be exported outside this package for now! |
There was a problem hiding this comment.
This is the most important line of code in this file 😁
There was a problem hiding this comment.
Great work @gaetanmaisse. 💯
Probably needs refinement, but will merge for now and we can pick that up between now and release, or down the road.
shilman
changed the title
Issue: Part of #5030 and supersede #9870
What I did
I migrated
@storybook/coreto TypeScript, some types still need to be refined but the base is here.A PR had been started in early 2020 about that (#9870) but never gets merged and has too many conflicts now so I just started from scratch here.
I think this is the final step of the TS migration for
libpackages 🎉How to test