-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
TypeScript: Re-structure types for frameworks and presets #18504
Conversation
…ch other Also map framework.options.builder into core.builder.options
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 83759ea. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
… in preset-react-webpack (it already was)
I'm thinking we should rename Maybe |
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.
Can you update examples to show how this is used?
@tmeasday I did update the examples' main.ts if needed, but most of them didn't change all that much: storybook/examples/official-storybook/main.ts Lines 2 to 4 in cc4a40b
storybook/examples/react-ts/.storybook/main.ts Lines 1 to 3 in cc4a40b
storybook/examples/cra-ts-essentials/.storybook/main.ts Lines 1 to 5 in cc4a40b
@tmeasday I'm working on a PR where I migrate all examples to use |
# Conflicts: # examples/svelte-kitchen-sink/.storybook/main.ts
type all the things for examples main & frameworks and presets
cleanup SB-444 examples
@shilman please re-review? |
cleanup (angular) SB-444
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.
Generally looks great. A couple of nitpicks. Sorry for the slow review!
export type StorybookConfig<TWebpackConfiguration = WebpackConfiguration> = | ||
StorybookConfigBase<TWebpackConfiguration>; | ||
|
||
export type SvelteOptions = { |
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.
How does this get associated with any of the config types? Or is that not necessary?
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.
It gets composed here:
storybook/frameworks/svelte-webpack5/src/types.ts
Lines 15 to 17 in 63460cf
export type FrameworkOptions = SvelteOptions & { | |
builder?: BuilderOptions; | |
}; |
# Conflicts: # docs/snippets/html/button-story-with-args.ts.mdx
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.
Looking good! Merging this
What I did
after discussion on #18432
core-common
➔core-webpack
➔preset-react-webpack
➔react-webpack5
And did this for all presets and frameworks.
At each level customizations are made to make types more strict
framework.options.builder
field, which is equal tocore.builder.options
This way users of a framework don't have to specify a builder if they want to configure something in the framework
DEMO:
Screen.Recording.2022-06-17.at.16.39.21.mov
I also changed all the main.JS files of all examples into a main.TS file and ensured it uses the type from the framework is for.
I removed the 6-0 and 7-0 types in renderers.
I changed all stories and example code to deal with the change in 7.0 where what used to be
Story
is nowStoryFn
I reorganized the renderers and frameworks to be simpler, and more in sync with each other. (there's still some work to do here actually.
To be precise, it seems the types for react are way, WAY better.