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

[Bug]: Investigate code getting pre-bundled multiple times #20514

Closed
shilman opened this issue Jan 6, 2023 · 8 comments
Closed

[Bug]: Investigate code getting pre-bundled multiple times #20514

shilman opened this issue Jan 6, 2023 · 8 comments

Comments

@shilman
Copy link
Member

shilman commented Jan 6, 2023

Describe the bug

Inside the new preview-api package things appear to be included too many times. For example:

shilman@MBP14 lit-vite-default-js % grep getSelectionSpecifierFromPath node_modules/@storybook/preview-api/dist/*
node_modules/@storybook/preview-api/dist/chunk-S6GMWV4D.mjs:var getSelectionSpecifierFromPath = () => {
node_modules/@storybook/preview-api/dist/chunk-S6GMWV4D.mjs:    this.selectionSpecifier = getSelectionSpecifierFromPath();
node_modules/@storybook/preview-api/dist/core-client.js:var getSelectionSpecifierFromPath = () => {
node_modules/@storybook/preview-api/dist/core-client.js:    this.selectionSpecifier = getSelectionSpecifierFromPath();
node_modules/@storybook/preview-api/dist/index.js:var getSelectionSpecifierFromPath = () => {
node_modules/@storybook/preview-api/dist/index.js:    this.selectionSpecifier = getSelectionSpecifierFromPath();
node_modules/@storybook/preview-api/dist/preview-web.js:var getSelectionSpecifierFromPath = () => {
node_modules/@storybook/preview-api/dist/preview-web.js:    this.selectionSpecifier = getSelectionSpecifierFromPath();

Note that this is also bundled in the preview package as well, further contributing to the install size.

This may contribute to a ballooning install size in our benchmarks:

image

To Reproduce

No response

System

No response

Additional context

No response

@ndelangen
Copy link
Member

These are all the commonJS files, which do indeed not tree-shake, and for backwards compatibility have all the old code still present/bundled.

What are we measuring here?

@ndelangen
Copy link
Member

I can't say with full confidence, but I think these commonjs files would only ever be used by storyshots, I think.

@ndelangen
Copy link
Member

I verified that the MJS files are in the prebundle.

@ndelangen
Copy link
Member

I have noticed that the deprecation warnings for the removed packages don't actually make it into the ESM dist (they do appear in the CJS dist)

@ndelangen ndelangen moved this from Required for RC to In Progress in Core Team Projects Jan 12, 2023
@ndelangen
Copy link
Member

I verified, that in the webpack-builder the externalisation of the packages we want work correctly, and they are all ESM.

@ndelangen
Copy link
Member

I found that vite-builder was using :

import externalGlobals from 'rollup-plugin-external-globals';

But this resulted in the code existing multiple times.
Switching to

import { viteExternalsPlugin } from 'vite-plugin-externals';

made it work!

@ndelangen
Copy link
Member

will open a PR.

@shilman
Copy link
Member Author

shilman commented Jan 13, 2023

w00t!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.0-beta.26 containing PR #20594 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb@next upgrade --prerelease

Closing this issue. Please re-open if you think there's still more to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

2 participants