Storybook: Improve TypeScript performance for slow stories #63388
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mirka
added
[Type] Performance
|
Size Change: -4.47 kB (-0.25%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
mirka
commented
Jul 11, 2024
.eslintrc.js
Outdated
Comment on lines
391
to
395
| files: [ '**/@(storybook|stories)/*' ], | ||
| rules: { | ||
| // Suggested workaround for hooks used in CSF3 format stories. | ||
| // https://github.com/storybookjs/eslint-plugin-storybook/pull/149 | ||
| 'react-hooks/rules-of-hooks': 'off', |
There was a problem hiding this comment.
In CSF3, a functional component can be defined under the render property of the story object. This does not look like a proper component to the rules-of-hooks linter, so it errors if you use a hook inside.
There was a problem hiding this comment.
You can simply pass a named function with a name that starts with uppercase and it will be recognized as a component. I do this in my own project. Could we try that?
I normally just go for function Render. See archetype-ui stories for reference, if you need to.
There was a problem hiding this comment.
Agreed. If we can avoid adding extra complexity to the ESLint configuration with a valid alternative syntax, let's do it.
There was a problem hiding this comment.
Sure, we can go with that! (Interestingly, most of the existing hooks were moved into decorator functions, which are anonymous and don't trigger the rule-of-hooks lint 😳)
Reverted in 24569a5
There was a problem hiding this comment.
Yep. Was going to say this. Just came here running a lint without the ESLint changes locally and was surprised that there were no errors.
| }; | ||
|
|
||
| export const Default = Template.bind( {} ); | ||
| export const Default: StoryObj< typeof ColorPicker > = {}; |
There was a problem hiding this comment.
Switched to an uncontrolled story for simplicity.
| export const Default: StoryObj< typeof DropdownMenu > = { | ||
| decorators: [ | ||
| ( Story ) => ( | ||
| <div style={ { height: 150 } }> |
There was a problem hiding this comment.
I noticed these explicit heights are no longer necessary due to #53889, because popovers are now rendering at the end of the document rather than within the each story div, which used to cut them off visually. I'll do a separate cleanup PR.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
tyxla
reviewed
Jul 12, 2024
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jul 18, 2024
…#63388) * Storybook: Improve TS performance for slow stories * Disable `rules-of-hooks` lint for stories (CSF3 support) * Fixup * Revert eslint changes * Fix unwanted composition in DropdownMenu story Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * Fixup --------- Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Optimizes some low-hanging bottlenecks in the TypeScript compilation performance for Storybook stories, as revealed by
@typescript/analyze-trace.Improvement in cold build time
Honestly the build times are not very stable so I'm not sure how much we can read into it, but the analyzed output of
@typescript/analyze-traceclearly shows improvements, so I think we can say that we're moving things in the correct direction.Why?
These particularly slow stories have TS performance issues in the components themselves, and we ultimately need to address the root cause. However, their Storybook stories can be optimized to "bypass" some of the particularly slow parts. Luckily, this mostly just involves converting the story format from CSF2 to CSF3, with a bit of attention given to minimizing unnecessary type checking work. And CSF3 is good for us to transition to anyway.
Testing Instructions
Smoke test the affected stories to see that there are no regressions.
Code review should be easier with whitespace hidden.