Vite: Export storybook utilities from frameworks for better pnpm support

[IanVS]

Issue: N/A

This is one step towards improved pnpm and yarn pnp support for vite-based storybook frameworks. It does this by moving the imports of various storybook utilities to an explicit user dependency, the frameworks.

What I did

The reason we had some problems in the past, is that even though the vite builder was depending on these packages, they're used in a virtual module in the browser, which sets up its own module resolution context. So, unless the user has those packages in their own package.json (or turns on hoisting), those utilities won't be at the root node_modules, and will error out.

This has the downside that we now have duplicated re-exports in all of the vite frameworks. We could clean this up by re-introducing a vite-core package, that handles it in one place, and then each of the frameworks can just export everything from core.

How to test

I attempted to test this in a sandbox, using pnpm and vercaddio, but I hit a strange snag:

Error: Cannot find module 'property-information/svg'

It seems this comes from a transitive dependency of mdx@1.

The other way I tested this out was by hacking on node_modules in a separate project, and it worked out alright there. But, pnpm won't work "out of the box" until #19176 is addressed. But it limits the number of packages that the user needs to install in their package.json to just addons, so it's a step in the right direction.

[IanVS]

The svelte sandbox errors seem to be related to the onClick arg not wiring up correctly to the svelte component in the UseState story. I think it would fail in svelte-webpack as well, but it looks like we don't have a sandbox for that.

And I think the react-vite snapshots are actually broken in the baseline but fixed now, so should be accepted.

[ndelangen]

@IanVS I fixed the svelte component..

@tmeasday I think you should take at this..

[shilman]

@IanVS Let's merge this as is, then potentially follow up with a refactor to get this into core-vite along with the utils from #19259?

[shilman]

@tmeasday WDYT about the feasibility of #19176 for 7.0?

[tmeasday]

Question: is this issue not also present in webpack also? I would imagine the same limitation about importing in virtual modules from non-direct dependencies in a WP build would apply.

I'm generally in agreement about this change but I'd suggest we do it in a slightly different way. Why don't we factor out this little chunk of code that's common to both https://github.com/storybookjs/storybook/blob/next/code/lib/builder-webpack5/templates/virtualModuleModernEntry.js.handlebars and https://github.com/storybookjs/storybook/blob/next/code/lib/builder-vite/src/codegen-modern-iframe-script.ts and export it as a single symbol from core-server? Then each framework just needs to re-export one thing?

If you compare those files they are quite similar, with some crucial differences.

I guess there are some other imports you are using in various vite codegen files. I'm thinking we need to do the same audit for the webpack virtual modules also though.

[tmeasday]

@tmeasday WDYT about the feasibility of #19176 for 7.0?

I think we should do it.

[IanVS]

Why don't we factor out this little chunk of code that's common to both next/code/lib/builder-webpack5/templates/virtualModuleModernEntry.js.handlebars and next/code/lib/builder-vite/src/codegen-modern-iframe-script.ts and export it as a single symbol from core-server? Then each framework just needs to re-export one thing?

That's an interesting thought. That could be a nice way to keep things DRY, though I worry a little bit about coupling webpack and vite together in a way that may cause troubles. Then again, the way things are now we run the risk of them drifting apart, so I guess it's a tradeoff.

As for this issue happening in webpack, it doesn't seem to have the same problem. I suspected as much after searching through issues for pnpm, and not finding anything similar to the ones we get in the vite-builder repo about hoisting. Then I tested it out, using npx -p @vue/cli vue create . --default --packageManager=npm --force --merge, deleting node_modules and package-lock.json, and then pnpm i && pnpm run serve, and the app loaded up just fine without any kind of hoisting config or installing extra packages. Webpack's way of bundling everything for the browser might help it in this regard, I guess.

Anyway, would you be opposed to taking an incremental approach here, continuing with this strategy for now, then cleaning up the duplication into a vite-core package or similar, and then maybe also DRYing it up with webpack in the future? I'd love to get pnpm into a better spot as soon as possible.

[shilman]

@tmeasday I'm going to merge & release this (if CI passes) to let @IanVS test this more easily. If we want to tackle it differently, let's iterate once this has been validated.

[IanVS]

I tested out alpha.34, and now in a pnpm svelte-vite project, I needed to only install the following packages manually:

	"@mdx-js/react": "1",
	"@storybook/addon-actions": "^6.5.12",
    "@storybook/addon-backgrounds": "^6.5.12",
    "@storybook/addon-docs": "^6.5.12",
    "@storybook/addon-highlight": "7.0.0-alpha.7",
	"@storybook/addon-measure": "^6.5.12",
    "@storybook/addon-outline": "^6.5.12",
    "@storybook/core-client": "^6.5.12",
	"react": "^18.2.0",
    "react-dom": "^18.2.0",

React and react-dom are peer-deps (for all frameworks, due to mdx, which is a bit confusing for users sometimes), and @storybook/core-client isn't used directly in builder-vite, but is one of the modules returned in the previewAnnotations from:

storybook/code/lib/core-server/src/presets/common-preset.ts

Line 96 in 1041236

return [...config, require.resolve('@storybook/core-client/dist/esm/globals/globals'), ...base];

[shilman]

cc @ndelangen ☝️

[tmeasday]

I guess the core-client issue would be resolved by #19176?

Still I think it's a bit weird that the code is in that package, which otherwise is only used for legacy v6 stuff. @ndelangen I'd really like to take some time to fix up the preview packages sometime soon.

[tmeasday]

@IanVS is there going to be any way to resolve the react thing? Or do we just make the CLI install react as part of sb init if using pnpm (or always?)

[IanVS]

I don't think it's pnpm specific, using MDX requires react, I believe. Although with MDX2, there are ways to render using other frameworks, like vue and preact https://mdxjs.com/packages/. Not all frameworks we support though, and I'm guessing it would be a lot of work to use them. @shilman has a better idea of that I'm sure.

Another idea would be to make mdx optional. Not every project needs it or wants it, and some teams might opt for a smaller and cleaner installation instead. But again, not sure how much work that is.

[tmeasday]

Yeah, I guess a sb init --no-mdx would make sense, not sure when that'll get done but hey 🤷

[IanVS]

I think core-client/globals/globals can just be deleted. It's only to add STORYBOOK_REACT_CLASSES, which seems to not even be used anymore, as far as I can tell.

I opened #19300 to start a discussion about it at least.