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

[canvas][storybook] Improve Storybook Performance #34757

Merged

Conversation

clintandrewhall
Copy link
Contributor

@clintandrewhall clintandrewhall commented Apr 8, 2019

Summary

Storybook performance has been pretty poor compared to my past experiences, so I started digging into the details. After chatting and filing an issue with the Storybook dev team, there was an issue with react-docgen-typescript which was fixed just days prior. I've included that change.

In addition, with the help of @spalger, I was able to offload almost all of the 10MB+ of code dependencies into a DLL, leaving only a couple of MB of Canvas and Storybook code behind.

All considered, this PR drops the Canvas Storybook build time from 1.5 minutes to 9 seconds.

Screen Shot 2019-04-05 at 8 40 56 AM

This PR also includes:

  • updates to the Kibana build_sass script to allow --watch;
  • fixes to the webpack config to ensure Canvas plugin CSS would be loaded and watched properly.

Testing

  • Run node scripts/storybook and ensure the DLL is built, CSS is watched, and Storybook starts properly.
  • Kill the script, then run node scripts/storybook again. Ensure the DLL is not build again.
  • Run yarn kbn bootstrap and ensure the DLL code is removed.
  • Run node scripts/storybook_static and ensure a static build of the Storybook site is produced.
  • Run node scripts/jest and ensure the Jest tests pass.

@clintandrewhall clintandrewhall added review Feature:Dev Tools Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.0.0 v7.2.0 labels Apr 8, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas

@clintandrewhall clintandrewhall requested a review from a team as a code owner April 8, 2019 21:57
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@clintandrewhall clintandrewhall changed the title [canvas][storybook] ImproveStorybook Performance [canvas][storybook] Improve Storybook Performance Apr 8, 2019
Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

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

Looks great. Everything works as advertised for me. Great work on this.

Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

Looks great, and super zippy! 🎉

@tylersmalley
Copy link
Contributor

tylersmalley commented Apr 10, 2019

Love the speed improvements!

There is already a watcher implemented for Kibana development used here: https://github.com/elastic/kibana/blob/master/src/legacy/server/sass/index.js#L82 It handles a few cases which are not covered, like watching dependencies outside the directory. Think you could generalize the code there for use in both the dev server and plugin build?

@@ -6,6 +6,7 @@
"license": "Elastic-License",
"scripts": {
"kbn": "node ../scripts/kbn",
"kbn:bootstrap": "rm -rf ../built_assets/canvasStorybookDLL",
Copy link
Contributor

@tylersmalley tylersmalley Apr 10, 2019

Choose a reason for hiding this comment

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

Curious why is this necessary? Shouldn't it get overwritten when it's built?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to force a rebuild of the DLL only if the packages change. We don't want to rebuild the DLL on every startup, just when the dependencies are updated.

log.verbose(styleSheetPaths);

if (watch) {
const debouncedBuild = debounce(async path => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there supposed to be a wait defined for the debounce here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... I'm not sure. I saw this methodology elsewhere in Kibana for a watch flag, so I took it as the best approach.

// Extend the Storybook Webpack config with some customizations
module.exports = async ({ config }) => {
// Include timing and other data in the Webpack build stats file.
config.profile = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to commit this? I am not sure what the overhead to profiling is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can take it, sure.

@tylersmalley
Copy link
Contributor

Left a couple of comments - In regards to re-using the existing SASS watcher, I am fine punting on this if you feel it's necessary at this time.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@clintandrewhall clintandrewhall merged commit 1bfcf50 into elastic:master Apr 15, 2019
clintandrewhall added a commit to clintandrewhall/kibana that referenced this pull request Apr 15, 2019
* [WIP] Initial commit

* [canvas][storybook] Improving Storybook Performance

* Adding docs; fixing bugs

* Address feedback; add todo
clintandrewhall added a commit that referenced this pull request Apr 15, 2019
)

* [canvas][storybook] Improve Storybook Performance (#34757)

* [WIP] Initial commit

* [canvas][storybook] Improving Storybook Performance

* Adding docs; fixing bugs

* Address feedback; add todo

* Delete stats.json
@clintandrewhall clintandrewhall deleted the storybook-improvements branch June 6, 2019 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dev Tools review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants