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

refactor: [M3-8184] - Improve local Storybook performance #10762

Merged

Conversation

hana-akamai
Copy link
Contributor

@hana-akamai hana-akamai commented Aug 8, 2024

Description 📝

Performance optimizations for running Storybook locally

The main culprit is using react-docgen-typescript; it's parsing every file for docgen info. The package is also no longer being maintained (the last release was in end of 2021).

  • There's a faster docgen library that Storybook 8 uses, react-docgen, however we're missing a lot of typing by using that since it's using a shallower analysis.
  • The solution I ended up at was to keep react-docgen-typescript, but tighten the scope on what files are parsed for the docgen (Components and Features only). And to also disable allowSyntheticDefaultImports and esModuleInterop per the Storybook MUI docs

The time from first paint to Intro loaded on my Intel i7 2019 MBP went from over 1 minute to 30s 🎉

Preview 📷

Before After
Screen.Recording.2024-08-08.at.11.49.20.AM.mov
Screen.Recording.2024-08-09.at.1.22.42.PM.mov

How to test 🧪

Reproduction steps

(How to reproduce the issue, if applicable)

  • On the develop branch, run Storybook locally and time the time it takes from first paint to Intro loaded

Verification steps

(How to verify changes)

  • Pull down this branch, run Storybook locally and the time from first paint to Intro loaded should be shorter than develop
  • The should be no regressions in the typing and loading of stories compared to https://design.linode.com/

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@hana-akamai hana-akamai added Storybook Performance Increase performance of CM labels Aug 8, 2024
@hana-akamai hana-akamai self-assigned this Aug 8, 2024
packages/manager/.storybook/main.ts Outdated Show resolved Hide resolved
packages/manager/.storybook/main.ts Outdated Show resolved Hide resolved
@hana-akamai hana-akamai marked this pull request as ready for review August 8, 2024 16:28
@hana-akamai hana-akamai requested a review from a team as a code owner August 8, 2024 16:28
@hana-akamai hana-akamai requested review from jaalah-akamai and coliu-akamai and removed request for a team August 8, 2024 16:28
Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was only about maybe a second difference for me and both were super quick, around 5 seconds, but I also have an M3 chip/macbook from 2023 😅

Saw these stats in the terminal though, which maybe corroborates?

  • Develop: 320 ms for manager and 4.02 s for preview (ranged about 3.5-5s for the preview)
  • this branch: 317 ms for manager and 2.8 s for preview (ranged about 2.5-3.5s for the preview)

Gonna approve, tho might be worth getting other people with older processors to see if they have a bigger difference? 😅


The package is also styleguidist/react-docgen-typescript#494 (the last release was in end of 2021).

Hm...should we potentially use a different package down the line/keep this in the back of our minds?

This means future features added to Storybook will also need to be added to the include array.

Could we document this somewhere for future features so it doesn't get lost? Repository structure and Component library both mention storybook. Maybe Component library would work better, although that's more about components than features - not too sure where the best place to mention this would be

Comment on lines +25 to +29
// Speeds up Storybook build time
compilerOptions: {
allowSyntheticDefaultImports: false,
esModuleInterop: false,
},
Copy link
Contributor Author

@hana-akamai hana-akamai Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found this upon further investigation storybookjs/storybook#24943 (comment)

Copy link

github-actions bot commented Aug 9, 2024

Coverage Report:
Base Coverage: 82.62%
Current Coverage: 82.62%

@jaalah-akamai
Copy link
Contributor

I didn’t notice a significant difference, so I believe we can dig deeper by profiling the initial load to identify what’s blocking the main thread. There seem to be two instances of long-running tasks, which could be related to how we're merging MUI themes (possibly). I would also see what's required to bump to 8.2 where they consolidated dependencies for better performance.

@hana-akamai
Copy link
Contributor Author

@jaalah-akamai The main performance issue is using react-docgen-typescript. The performance improvement was negated in 792fc7d due to including all feature files.

I dug a bit deeper to see if we can automatically tell react-docgen-typescript which files to check without having to specify everything under the features folder and without having to manually do it. That has been addressed in 83e5970.

I did not notice a significant difference by upgrading to 8.2, and upgrading came with a bunch of false positive logging that is a known issue so will hold off on the update until the Storybook team addresses that

storybook 8 2 7 missing source file

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like my approval from last time is still there 😆

Also similar to last time, both loading times were pretty quick and there wasn't too big a difference for me. I'm definitely curious about the comment you left on packages/manager/.storybook/main.ts. Hopefully we can find a solution - will try to look into it a bit too!

@dwiley-akamai dwiley-akamai self-requested a review August 19, 2024 20:46
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My machine has the same processor (2.6 GHz 6-Core Intel Core i7) and I got the following results for the first paint --> intro loaded time:

develop:
Screenshot 2024-08-19 at 4 57 15 PM

This branch:
Screenshot 2024-08-19 at 5 03 36 PM

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing similar performance on develop when I run yarn storybook when the cache is warm.

develop this PR
develop-storybook-perf.mov
this-pr-storybook-pref.mov
~22 seconds ~22 seconds

This is just preference, but I don't like adding extra configuration unless it brings significant value because it gives me flashbacks to our webpack configs 😅

Happy to approve if I'm able to observe significant performance improvement, but I haven't been able to replicate when the cache is warm (meaning yarn storybook has been run at least once before).

I didn't do extensive testing when the cache is cold / does not exist, but again, I'm not sure extra configuration is worth better performance when it only applies to cold starts

I don't want my preference/opinions to block this, I just want to make sure the change actually delivers value for the team.

packages/manager/.storybook/utils.ts Outdated Show resolved Hide resolved
@hana-akamai
Copy link
Contributor Author

@bnussman-akamai Removed the vite config update since that didn't bring much value. Personally, I notice a ~10s improvement on my machine for cold starts. I don't run Storybook that often so when I do it's usually a cold start but curious to hear how others use Storybook

@hana-akamai hana-akamai merged commit 429ed60 into linode:develop Aug 27, 2024
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance Increase performance of CM Storybook
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants