Compile addons/lib in both CJS/ESM + Enable lazy loading for some larger components

[hipstersmoothie]

Issue: adresses some of #9290 (polyfills still included), #9282

What I did

The bundle size for included dependencies in a storybook website can get quite large. This PR lays the groundwork to incrementally improve the state of tree-shaking and lazy-loading in storybook's output.

This PR main focuses on:

• Compile all addons and lib package into both CJS and ESM versions
• Find large components and lazy load them (ColorControl, react-syntax-highlighter, tooltip)

Addon Changes

The biggest hurdle in making this work was getting the addons to work.

Currently an addon package is a mix of two types of code:

1. Code ran in node that builds the storybook webpack configs (preset and register code)
2. Code ran from within the storybook. This code is ran through webpack and should be tree shaken.

Since the addon dist/ folder now has two sub-folders (cjs and esm). Defining the preset code in src led to hard pathing problems. To mitigate this I chose to move all of the preset code straight into the root preset.js file. From this file you can still access compiled utils in dist/cjs, but all entries and other files loaded into storybook are from dist/esm.

This is not a breaking change to the API, just a better way to structure storybook's addon code.

Upgrade Babel + TypeScript

Exporting ESM code exposed a lot of places in code where we were explicitly exporting a type or interface. To fix this I change those to export type statements. This required upgrading to [email protected] and [email protected].

Is this a breaking change?

This does not break an APIs but it does break direct import from dist (ex: @storybook/links/dist/react). Whether this breaks APIs is up to the maintainers 😄 . In some of these cases I just chose to include whatever was being imported to the index file. This is both better for knowing what the public api is and is simpler for the user.

Possible Further Improvements

• Lazy load more of the addon code (ex: a11y, docs, React.Lazy the panel code)
• Deprecate register.js files as they break tree-shaken

Results

examples/react-ts before:

examples/react-ts after:

How to test

• Is this testable with Jest or Chromatic screenshots? yes
• Does this need a new example in the kitchen sink apps? no
• Does this need an update to the documentation? no

If your answer is yes to any of these, please make sure to include it in your PR.

[shilman]

@hipstersmoothie you are a superhero for taking this on, and this PR is epic 🤯

any chance you can get the build passing while we review?

[hipstersmoothie]

@shilman this is as ready as I can get it without some guidance

[shilman]

Amazing @hipstersmoothie. I'll try to make sense of all this today with @ndelangen when he comes online

[shilman]

OK, I took a look with @ndelangen and we're both thrilled about this PR.

CI. The build error in Chromatic is due to IE11 compat breaking. Apparently the generated bundle has arrow functions:

decorators&&decorators.forEach(decorator=>Object(client_api.b)(decorator,!1))}

Perf I'm building out benchmarks for Storybook and the benchmark values are really bad on next right now due to a regression introduced late last week. I'm going to fix that now and then update this PR. I want to do an apples-to-apples perf comparison on this PR to see what it does to our numbers.

Review The PR is too big to review in its current form. What I propose is let's get the build working and the perf numbers, and then once we're happy, split it into two PRs (yes, I know 😅 🔫 ), one for the ESM stuff and the second for lazy loading.

WDYT?

[hipstersmoothie]

I can do that I but the lazy loading pr will only be like 6 files so it's not gonna make the burden of review to much smaller

Edit: I lied. The lazy loading touches 10 files 😅

the main problem is that to get tree shaking working at all I needed to changes the builds for every package that uses the thing we want tree shaken. So if anything is using a component in from cjs compiled files, everything gets included and no tree shaking happens.

[shilman]

Ok hold off on splitting then. Let's discuss again after i get the perf stuff in hand

[hipstersmoothie]

@shilman I already cleaned up the commits. 1st commit is all the esm stuff, 2nd commit is all the lazy loading stuff.

Just fixed the IE11 stuff too. I had to stop transpiling the virtual module templates to get the tree shaking to work (import/export need to be in the file for it to work).

8595a7e

[shilman]

@hipstersmoothie I merged a higher-performing next baseline into this branch, but seem to have broken something. I don't understand the break--can you please take a look? 🙏

BTW, the new commit structure is 💯 . looks like something we can review!

[hipstersmoothie]

@shilman this PR actually fixes a lot of the issues that were fixed in that higher performing branch.

The following pattern is quite abundant in the storybook code:

1. Add code to a package but don't expose it in the main export
2. Make a file + type definition at root of package that points to that file creating 2 API entrypoints for a package (ex: @storybook/source-loader/extract-source, @storybook/components/html, etc)
3. Write docs to point people towards these APIs

Problems:

• We can't generate the types
• Relies solely on documentation to expose the API
• Harder to manager what constitutes a breaking change to a package

Benfits:

• Some code doesn't get included in bundle

By adding and ESM build to all of the packages and addons this fixes this problem. Since the ESM packages can actually be tree shaken now, perf regressions like the one you just fixed never have to happen. With that we can stop adding these extra root files and really depend on webpack to do its magic.

With ESM enabled we can now:

1. Add code to a package and export it from the index.js of that package
2. That's it!

And now all of our problems get fixed:

• We can't generate the types => Types are generated like normal
• Relies solely on documentation to expose the API => importing from the package suggests all the things it can do
• Harder to manager what constitutes a breaking change to a package => It's all in one file so it's super easy

And we still get

• Some code doesn't get included in bundle

But some problems do arise:

• It's hard to mix code that should run before webpack and after webpack in the same package (this is why I chose to move the preset code to the root. Since all the preset really does is provide some paths for webpack we point straight to the ESM code. Doing this from compiled src code is tricky)

[shilman]

Ok, I'm still wrapping my head around all this, so thank you for all the clarifications. This is tremendous work and I appreciate it even more as I dig in.

Here are some initial comments for discussion (cc @tmeasday @ndelangen @ghengeveld):

1. AFAICT this is a breaking change, since it is getting rid of the non-root entry points of the packages (e.g. @storybook/addon-docs/blocks). I'm not 100% against breaking changes (perf is top priority and I'm happy to release 7.0 if we can make a step change in perf).

2. Is it possible that we get rid of CJS entirely if this is already a breaking change? There are only a few cases where we need CJS as I understand it (code required by the presets)

3. There are some addon-docs README files deleted in this PR, and we'll need to move that content somewhere. I can take care of this with @jonniebigodes, but just want to make note of it here before we merge this.

4. Do we need to instruct third party addons to compile this way, or is it fully backwards compatible? What about stuff outside of lib/addons? Follow-on work for further optimization?

5. There is some special case code for virtualModuleEntry.template.js but none for virtualModuleStory or virtualModuleRef. Should there be?

Apologies in advance if these are stupid questions!

[hipstersmoothie]

1. For these I could just point the files to the esm code I think. so I don't necessarily believe it has to be a breaking change I think would need to test a little
2. I think this also would be possible I'll toy around with it too
3. Ah sorry about that! I noticed the actual js files weren't used and should have been removed for 6.0 so I went ahead and did it. I can add back the docs though
4. This in no way changes what happens for 3rd part addons. They def should ship esm code too for good tree shaking (will be doing this for addon-jsx). So everything should still work. Outside of the addons? I think all that's left is the app/ folder and all of that code seems pretty necassary, so no much tree shaking would happen anyway. Could be a place for further improvement though. Biggest bang for our buck would def be in the addons though
5. Both those files only import and use 1 thing so no tree shaking would happen. I initially did this because I think I saw that it was the cause of extra code in the bundle. will check this again tomorrow

And these are great questions!

[shilman]

Ok, I've also created a benchmark to track performance across PRs. I'll be documenting this more soon, but basically it's installing and running sb init against a fresh CRA install for each commit and measuring some KPIs.

It's not very user-friendly yet, but you can click on the "branch" drop-down to isolate different branches. Here's what it looks like for this PR:

[hipstersmoothie]

That's useful. Does it update as a push?

[hipstersmoothie]

Also could I see where this code that generates the benchmark is?

I think that I might need to tweak the webpack mode for this

[shilman]

The benchmark code is here: https://github.com/storybookjs/bench

The runner is here: https://github.com/storybookjs/storybook/blob/next/scripts/run-e2e-config.ts#L200-L209

It runs as part of our e2e tests on every push.

Feedback welcome on any of this. I plan to add another benchmark for a loaded Storybook, possibly @storybook/design-system or some other public Storybook.

[hipstersmoothie]

Updates

1. Still TODO

2. Is it possible that we get rid of CJS? I think the best path forward for easy usage is to ship both. This way if people are using bits of storybook code in their tests they don't have to worry about configuring jest to compile it. Or if someone is compiling for older targets. Less config is better in my opinion. While this may bloat the install size a little bit I think it's a fair tradeoff.

3. Still TODO

4. No action required

5. "There is some special case code for virtualModuleEntry.template.js" I just checked an this is for sure needed. The reason is mainly because of addon-docs

• addon-docs configs require a host of heavy things (syntax highlighting, source extraction)
• If the files does get transformed to CJS then tree shaking breaks and includes all the heavy stuff

With a CJS virtual module template:

with an esm virtual module template:

[hipstersmoothie]

@shilman

not sure why these particular metrics are so bad. Does this graph take the latest build? I know I just pushed commits that reduce the time. It would be super nice to be able to run the benchmark locally for testing. Not really sure how. I compared the startup time of the react-ts example and the changes in startup time was negligible

[shilman]

@hipstersmoothie running the benchmark locally is easy:

yarn create react-app cra-bench
cd cra-bench
npx @storybook/bench 'npx sb init'

this will dump the results in bench.json and bench.csv

unfortunately, it uses the latest version of storybook and not your local branch. to do that in CI we use verdaccio. i believe you can do this in the storybook repo:

yarn bootstrap --core
yarn local-registry --publish
yarn local-registry --port 6000 --open
yarn wait-on http://localhost:6000
yarn config set registry http://localhost:6000/

I haven't verified the registry part, but you can see the code here https://github.com/storybookjs/storybook/blob/next/.circleci/config.yml#L108-L137

[shilman]

I've set the "data freshness" in Google Data Studio to 15 minutes. That means that the data should be visible on the report about 45 minutes after you push a commit--not great, but it's what we've got for now.

I've updated the report to make it a little more user friendly. If you work in the Storybook repo (not a fork) and you have a branch called perf/xxx, it will show up in the new "perf branches" report pages automatically.

[shilman]

BTW if you didn't see it, the "static build" preview render times are a big improvement. 💪

[shilman]

LMK if you have any reporting requests. happy to modify/add pages, or can give you write access to the report if you'd like to edit that.

Also feel free to jump on our discord if you'd like to chat in realtime: https://discord.gg/UUt2PJb

[stale]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

[shilman]

This was extended and merged in #13013. Closing for now. Thank you @hipstersmoothie for helping get Storybook onto ESM!