Builder breaks with package managers that don't hoist (e.g. pnpm) #55
Comments
|
Should these be added in peer dependencies? |
|
Could it be because of this change? #42 |
Peer dependencies are a dumpster fire, it would be great to avoid them. I think part of the issue is that we want this builder to work with every framework, but we don't want to bundle all of the various framework plugins with it, some of which can break others (the issue #42 was trying to fix). I'm wondering more and more whether we need some kind of architecture which supports plugins to the builder, for the various frameworks, similar to how storybook itself works. It causes an explosion in the number of packages, but it would be much more reliable, I think. |
|
Why is |
|
It's used by |
eirslett
added
bug
|
I tagged this with "help wanted"; I don't use pnpm myself, so it would be nice if anybody who uses pnpm in their daily work has time to have a look! |
|
I had a quick look today and it was a bit of a headache. Issue is, it's difficult to use So this is all still broken. Are there any vite maintainers we can bring on to help? const resolve = require('resolve');
// We need Vite to precompile these dependencies, because they contain non-ESM code that would break
// if we served it directly to the browser.
module.exports.optimizeDeps = function optimizeDeps(options) {
return {
include: [
'airbnb-js-shims',
// etc...
].filter((m) => {
try {
resolve.sync(m, { basedir: options.configDir });
return true;
} catch (err) {
return false;
}
}),
}};I also wrote a plugin to make resolution of const { resolveRecursively } = require('./resolveRecursively');
const importsToMap = {
'@storybook/core-client': ['@storybook/core'],
'@storybook/client-logger': ['@storybook/core', '@storybook/core-client'],
'@storybook/client-api': ['@storybook/core', '@storybook/core-client'],
}
module.exports.storybookImports = function storybookImports(options) {
const { framework, frameworkPath, configDir } = options;
const frameworkImportPath = frameworkPath || `@storybook/${framework}`;
return {
name: 'storybook-imports',
config(id) {
return {
resolve: {
alias: Object.entries(importsToMap).map(([key, value]) => ({ find: key, replacement: resolveRecursively(configDir, frameworkImportPath, ...value, key, ) }))
},
optimizeDeps: {
include: Object.keys(importsToMap),
}
}
},
};
};Where this is const readPkgUp = require('read-pkg-up');
const { dirname } = require('path');
const resolve = require('resolve');
/**
* Continuously resolves packages relative to the previous module, starting from base
*/
const resolveRecursively = (base, ...entries) => {
const absolutePath = entries.reduce(
(basedir, entry) =>
resolve.sync(entry, { basedir, preserveSymlinks: false }),
base
);
// we get the location of the package.json so vite can use it's resolution,
// as opposed to Node's which will pick up the commonjs bundle (as opposed to esm if it exists)
const packageJson = readPkgUp.sync({ cwd: absolutePath });
return dirname(packageJson.path);
};
module.exports = { resolveRecursively }; |
|
Can you try running |
|
@eirslett that works, yep This makes sense, given that it's consistent with yarn/npm's hoisting behaviour. It's not really a viable workaround, as removing hoisting is one of the benefits of pnpm. |
|
I found the offending commit: dde6b5f, AKA #28 Basically, as the root used to be inside So we want some optimized deps/import to be relative to storybook-builder-vite and the rest be relative to the project root. Not sure how to accomplish that, though 😅 In the meantime, I've added this to my And it works! |
|
Just to attach some complexity to this issue, if you are using pnpm with (rushjs)[https://rushjs.io/pages/intro/get_started/] you don't have the option to use --shamefully-hoist |
|
Does it work with Vite 2.4.1 (or newer)? |
@eirslett I just tried Vite 2.4.1 and I get the same issue. I've updated the reproduction repo with the latest version of Vite and Storybook now.
@Bjodol I'm in the same boat with Rush, you probably don't want to enable shamefully-hoist even if you could. I'd suggest the workaround in #55 (comment) |
|
@Pinpickle Your workaround seems not to work within When running info => Loading presets
ERR! Error: [vite-plugin-mdx] "react" must be installedDo you have any idea for an easy workaround? |
|
@fdc-viktor-luft that doesn't sound good! Truth be told, I've stopped using storybook because even with this working, it was still painfully slow. I'm not sure why this error would come up now, when it didn't come up before. I think the real solution here would be to fix this builder, so the root can correctly be set to the project root (and React be installed). On another note, Vite's support of nested |
|
@Pinpickle Thanks for your response.
Do you have a working alternative or did you just do a simple "storybook" yourself instead? |
|
@fdc-viktor-luft I rolled my own, yeah. I only used storybook for development so I just needed an easy way to look at components with specific props. My code is pretty unique to my setup, but the main magic is using glob imports, i.e. |
|
@Pinpickle Nice 👍 Thanks for the hint 🤩 Sadly we're using Storybook in production, too, and I wanted to migrate all of our Frontend-Setup from CRA to Vite. The storybook is currently the only blocker 😖 I think I'll need to run CRA for Storybook and use Vite everywhere else as long as this will not be resolved. |
|
@Pinpickle Thx a lot for your suggestion. |
|
I'm still struggling with this. Even a simple Turning on Here's another example: https://github.com/robcaldecott/pnpm-storybook-builder-vite |
|
I'm not a pnpm expert by any means, but I've been working on getting the original reproduction above working, and after a lot of hacking around, I think I've done it. I pushed up the changes to a new repo: https://github.com/IanVS/vite-storybook-pnpm-shame This was a combination of adding a lot of new |
I think it is in some part due to Storybook and it's use of peerDependencies. I hear Storybook 7.0 is planning to address this. |
|
I've fixed |
just tested it, and it seems to work. |
|
That might be a suitable work around, but not a real solution 🤔 https://pnpm.io/npmrc#public-hoist-pattern
|
|
FWIW, we are working on improved pnpm support in 7.0, and our goal is to not require any hoisting or workarounds. |
can't wait to see this final solution shipped with a release version! |
Added storybook using pnpm with set hoist pattern - see [storybookjs/builder-vite#55 (comment)]
Added storybook using pnpm with set hoist pattern - see [storybookjs/builder-vite#55 (comment)]
Added storybook using pnpm with set hoist pattern - see [storybookjs/builder-vite#55 (comment)]
🎉 Thanks a lot @srflp for this solution ! works perfectly without clogging package.json or vite config.optimizeDeps.include with a bunch of storybook packages
This solution to change root appeared to work for me in the first time but was actually breaking |
|
@IanVS v7 is almost there as it's in Beta already? Is it going to be fixed for the release? |
|
Yes, the 7.0 beta works correctly with pnpm and yarn pnp. I'd recommend trying it out. You can upgrade with |
|
Doesn't much for me. For some reason I have issues with type exports all over the place. Thought that's the reason. |
|
@RIP21 feel free to hop into discord.gg/storybook and tag me there, or find me on mastodon at @IanVS@fosstodon.org. Or open an issue in the storybook repo and tag me there, and we can dig in. |
|
@RIP21 It works perfectly fine for me, have a look here https://github.com/kaminskypavel/fullpower-stack/tree/master/packages/ui @IanVS did a great job !👏 |
|
@kaminskypavel you don't have any type export it seems in it. |
Hi! It's still not working for me after running I'm running Using |
|
@omarelb I think this may be a regression from v7.0.0-beta.30. I'm having this issue as well, but it was working perfectly before that prerelease. See this and original issue. You could downgrade to v7.0.0-beta.29, but as mentioned in the original issue, there are some issues with HMR on Vite-based Storybooks (I'd personally downgrade to v7.0.0-beta.25) |
|
Thanks for the info @Dschungelabenteuer and FYI @omarelb, this is also working and a little less generic: |
|
This seems to have been resolved with Storybook 7 release for us. |
|
i have tried to get the vite builder working with storybook v7.0.2, v7.0.6, v7.0.7, and v7.1.0-alpha.8, and i still cannot get it to work in a monorepo using yarn v3.3.0 and pnp. first i needed to added packageExtensions:
'@storybook/core-common@*':
dependencies:
'@storybook/react-vite': '^7.1.0-alpha.8'
'react-dom': '^18.2.0'after adding that, running yarn, then running i also tried adding a import { dirname } from 'path';
import { mergeConfig } from 'vite';
import type { StorybookConfig } from '@storybook/react-vite';
const config: StorybookConfig = {
framework: {
name: '@storybook/react-vite',
options: {},
},
// ...
async viteFinal(config) {
// Merge custom configuration into the default config
return mergeConfig(config, {
root: dirname(require.resolve('@storybook/builder-vite')),
server: { fsServe: undefined },
});
},
};
export default config;when i run so i’m guessing the |
|
@acusti would you be willing to create a minimal reproduction and open a new issue so we can dig into it more there? |
|
@IanVS ok, i can do that when i get a free moment |
|
@IanVS sorry i never got around to working on a minimal repro, but i did return to trying to upgrade my yarn v3.x monorepo to storybook v7.x and the vite builder, and i wound up getting everything to work. it turned out that the errors i was seeing were mostly just symptoms of missing dependencies. in the end, i just needed to thoroughly configure the storybook workspace’s
|
|
Two years later I've finally tried this again and it works out-of-the-box with my Vite and pnpm setup. Storybook team: thanks for the hard work improving compatibility! |
and others
I recently tried to upgrade from version 0.0.8 to 0.0.10 on my repo, which is using pnpm as its package manager.
pnpm does not hoist dependencies (so, for example, storybook's dependencies are not available in the project's top level node_modules).
When starting a project with storybook-builder-vite and pnpm, we see this error:
Error: Failed to resolve force included dependency: airbnb-js-shims, which is one of the forced optimized dependencies here: https://github.com/eirslett/storybook-builder-vite/blob/main/packages/storybook-builder-vite/optimizeDeps.jsI'm confused because:
airbnb-js-shimswas in that list thererequire.resolve("airbnb-js-shims")shouldn't work from this file in pnpm, based on my understanding of node module resolution and pnpm. It is resolving, so something is wrong with my understandingManually installing airbnb-js-shims fixes this error, but it is replaced with the next one down the list.
Reproduction here: https://github.com/Pinpickle/storybook-builder-vite-0.0.10-pnpm-bug (you can run it under pnpm and yarn to see it breaking and working)
The text was updated successfully, but these errors were encountered: