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

Builder breaks with package managers that don't hoist (e.g. pnpm) #55

Closed
Pinpickle opened this issue Jun 29, 2021 · 50 comments
Closed

Builder breaks with package managers that don't hoist (e.g. pnpm) #55

Pinpickle opened this issue Jun 29, 2021 · 50 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@Pinpickle
Copy link

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.js

I'm confused because:

  • this used to work in 0.0.8 and airbnb-js-shims was in that list there
  • require.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 understanding

Manually 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)

@schalk-b
Copy link
Contributor

Should these be added in peer dependencies?

@eirslett
Copy link
Collaborator

Could it be because of this change? #42

@IanVS
Copy link
Member

IanVS commented Jun 29, 2021

Should these be added in peer dependencies?

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.

@IanVS
Copy link
Member

IanVS commented Jun 29, 2021

Why is airbnb-js-shims included in the list of optimize deps? Do we know what is trying to pull it in?

@eirslett
Copy link
Collaborator

It's used by @storybook/core-client. (You'll find it in the lockfile.)

@eirslett eirslett added bug Something isn't working help wanted Extra attention is needed labels Jun 29, 2021
@eirslett
Copy link
Collaborator

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!

@Pinpickle
Copy link
Author

I had a quick look today and it was a bit of a headache. Issue is, it's difficult to use optimizeDeps.include relative to another dep, it's only relative to the root. I managed to do resolution in optimizeDeps relative to the project root (using the package resolve) but then it just filters out heaps of packages and the browser breaks when trying to import.

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 @storybook/client-api relative to the framework installed at the root, although this was never broken:

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 resolveRecursively.js

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 };

@eirslett
Copy link
Collaborator

Can you try running pnpm install --shamefully-hoist, does that work?

@Pinpickle
Copy link
Author

@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.

@Pinpickle
Copy link
Author

Pinpickle commented Jun 30, 2021

I found the offending commit: dde6b5f, AKA #28

Basically, as the root used to be inside storybook-builder-vite, the resolutions in optimizeDeps are relative to the root. Now that the root is in the project itself, these resolutions aren't necessarily correct.

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 viteFinal to basically undo that commit:

const { dirname } = require("path");

// https://github.com/eirslett/storybook-builder-vite/issues/55
config.root = dirname(require.resolve("storybook-builder-vite"));
config.server.fsServe = undefined;

And it works!

@Bjodol
Copy link

Bjodol commented Jul 7, 2021

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

@eirslett
Copy link
Collaborator

eirslett commented Jul 7, 2021

Does it work with Vite 2.4.1 (or newer)?

@Pinpickle
Copy link
Author

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.

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

@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)

@vikingair
Copy link

vikingair commented Sep 7, 2021

@Pinpickle Your workaround seems not to work within pnpm workspaces. Transforming this repository into pnpm workspace and adding the above workaround allows to reproduce the following issue. (See #92)

When running cd packages/example-react && pnpm storybook you get the following error:

info => Loading presets
ERR! Error: [vite-plugin-mdx] "react" must be installed

Do you have any idea for an easy workaround?

@Pinpickle
Copy link
Author

@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 optimizeDeps.include (landed in 2.5.2, vitejs/vite#4634) should make this easier to fix properly.

@vikingair
Copy link

@Pinpickle Thanks for your response.

I've stopped using storybook

Do you have a working alternative or did you just do a simple "storybook" yourself instead?

@Pinpickle
Copy link
Author

@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. import.meta.glob("./**/*.stories.tsx")

@vikingair
Copy link

@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.

@zheeeng
Copy link

zheeeng commented Oct 13, 2021

@Pinpickle Thx a lot for your suggestion.

@robcaldecott
Copy link

I'm still struggling with this. Even a simple pnpm project (without using workspaces) fails to work without complaining about @mdx-js/react even with these various workarounds via viteFinal. It doesn't even appear to be running anything inside viteFinal before it gives up btw.

Turning on node-linker=hoisted in .npmrc fixes it but it would be good to find another workaround.

Here's another example: https://github.com/robcaldecott/pnpm-storybook-builder-vite

@IanVS
Copy link
Member

IanVS commented Mar 17, 2022

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 optimizeDeps.include that are sub-dependencies (which we can add to our own list in this project so others don't need to), but a few of them required adding the dependencies directly to the repo, which I don't understand and don't like. I'm not sure if this is a vite bug, or weird pnpm thing, or what. But at least it seems possible to get this working without hoisting.

@virtuoushub
Copy link

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 optimizeDeps.include that are sub-dependencies (which we can add to our own list in this project so others don't need to), but a few of them required adding the dependencies directly to the repo, which I don't understand and don't like. I'm not sure if this is a vite bug, or weird pnpm thing, or what. But at least it seems possible to get this working without hoisting.

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.

@shilman
Copy link
Member

shilman commented Apr 2, 2022

I've fixed addon-docs peer dependencies (the major culprit, but possibly not everything?) in 6.5 if anybody wants to take another look at this.

npx sb upgrade --prerelease

@kaminskypavel
Copy link

I've fixed addon-docs peer dependencies (the major culprit, but possibly not everything?) in 6.5 if anybody wants to take another look at this.

npx sb upgrade --prerelease

just tested it, and it seems to work.
unfortunately, it creates a new issue related to deduping @storybook/client-api.
the solution here seems to help though

@vikingair
Copy link

That might be a suitable work around, but not a real solution 🤔

https://pnpm.io/npmrc#public-hoist-pattern

This setting is useful when dealing with some flawed pluggable tools that don't resolve dependencies properly.

@IanVS
Copy link
Member

IanVS commented Oct 10, 2022

FWIW, we are working on improved pnpm support in 7.0, and our goal is to not require any hoisting or workarounds.

@rogerleung0411
Copy link

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!

Indeedornot added a commit to Indeedornot/SvelteToDo that referenced this issue Oct 26, 2022
Indeedornot added a commit to Indeedornot/SvelteToDo that referenced this issue Oct 27, 2022
Indeedornot added a commit to Indeedornot/SvelteToDo that referenced this issue Oct 27, 2022
@williamgoulois
Copy link

williamgoulois commented Oct 28, 2022

This single line in .npmrc fixed my problems with storybook and npm:

public-hoist-pattern[]=*storybook*

🎉 Thanks a lot @srflp for this solution ! works perfectly without clogging package.json or vite config.optimizeDeps.include with a bunch of storybook packages
Don't forget to run pnpm install to update node_modules structure after your changes

config.root = dirname(require.resolve("storybook-builder-vite"));

This solution to change root appeared to work for me in the first time but was actually breaking @storybook/addon-postcss.
The CSS file that i was importing in preview.tsx with @tailwind directives was picked up but tailwind styles were not applied.
HMR on modification of a component classname did not trigger an update of the CSS file too.

@RIP21
Copy link

RIP21 commented Dec 16, 2022

@IanVS v7 is almost there as it's in Beta already? Is it going to be fixed for the release?

@IanVS
Copy link
Member

IanVS commented Dec 16, 2022

Yes, the 7.0 beta works correctly with pnpm and yarn pnp. I'd recommend trying it out. You can upgrade with npx sb@next upgrade --prerelease.

@RIP21
Copy link

RIP21 commented Dec 16, 2022

Doesn't much for me. For some reason I have issues with type exports all over the place. Thought that's the reason.

@IanVS
Copy link
Member

IanVS commented Dec 16, 2022

@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.

@kaminskypavel
Copy link

@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 !👏

@RIP21
Copy link

RIP21 commented Dec 16, 2022

@kaminskypavel you don't have any type export it seems in it.
I have 1:1 config to yours aside from vite-tsconfig-paths. But even without it and fixed paths, it fails with exactly the same issue.
For reference here is the repro
https://github.com/RIP21/sb7-vite-bug-repo
SyntaxError: The requested module '/src/components/tooltip/index.ts' does not provide an export named 'TooltipProps'
Error is this one.

fufuShih added a commit to fufuShih/storybook-react-typescript-vite-mui that referenced this issue Dec 29, 2022
@omarelb
Copy link

omarelb commented Jan 19, 2023

Yes, the 7.0 beta works correctly with pnpm and yarn pnp. I'd recommend trying it out. You can upgrade with npx sb@next upgrade --prerelease.

Hi! It's still not working for me after running npx sb@next upgrade --prerelease. I'm getting the following error:

[vite] Internal server error: Failed to resolve import "@storybook/preview-api" from "../../../../../../../../virtual:/@storybook/builder-vite/vite-app.js". Does the file exist?
  Plugin: vite:import-analysis
  File: /virtual:/@storybook/builder-vite/vite-app.js
  1  |  import { composeConfigs, PreviewWeb, ClientApi } from '@storybook/preview-api';
     |                                                         ^
  2  |    import '/virtual:/@storybook/builder-vite/setup-addons.js';
  3  |    import { importFn } from '/virtual:/@storybook/builder-vite/storybook-stories.js';

I'm running pnpm in a monorepo and trying to get storybook running for one of the subpackages. In a pnpm monorepo the actual node_modules assets are located in the root folder. Maybe that has something to do with the issues? Here are the dependencies:

    "@storybook/addon-actions": "^7.0.0-beta.30",
    "@storybook/addon-essentials": "^7.0.0-beta.30",
    "@storybook/addon-links": "^7.0.0-beta.30",
    "@storybook/web-components": "^7.0.0-beta.30",
    "@storybook/web-components-vite": "^7.0.0-beta.30",
    "storybook": "^7.0.0-beta.30",
    "@babel/core": "^7.20.12",
    "babel-loader": "^8.3.0"

Using public-hoist-pattern[]=*storybook* as suggested by others does work.

@Dschungelabenteuer
Copy link
Member

Dschungelabenteuer commented Jan 19, 2023

@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)

@vikingair
Copy link

Thanks for the info @Dschungelabenteuer and FYI @omarelb, this is also working and a little less generic:

public-hoist-pattern[]=@storybook/*

@abencun-symphony
Copy link

This seems to have been resolved with Storybook 7 release for us.

@IanVS IanVS closed this as completed Apr 24, 2023
@acusti
Copy link

acusti commented Apr 25, 2023

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 to my .yarnrc.yml to resolve Error: @storybook/core-common tried to access @storybook/react-vite, but it isn't declared in its dependencies; this makes the require call ambiguous and unsound. and Error: @storybook/react-dom-shim tried to access react-dom (a peer dependency) but it isn't provided by its ancestors; this makes the require call ambiguous and unsound.:

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 storybook dev, i am able to get the info => Starting manager... log with the info box saying that Storybook started and showing URLs to access it before i see the following error:

[vite] Internal server error: Failed to resolve import "@storybook/react/preview" from "../../../../../../virtual:/@storybook/builder-vite/vite-app.js". Does the file exist?
  Plugin: vite:import-analysis
  File: /virtual:/@storybook/builder-vite/vite-app.js:7:47
  5  |
  6  |    const getProjectAnnotations = async () => {
  7  |      const configs = await Promise.all([import('@storybook/react/preview'),
     |                                                ^
  8  |  import('@storybook/addon-essentials/docs/preview'),
  9  |  import('@storybook/addon-essentials/actions/preview'),

i also tried adding a viteFinal item to my .storybook/main.ts config (and a direct devDependency on @storybook/builder-vite v0.4.2):

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 storybook dev with that config, i see the info => Starting manager... log, then (before the Storybook started info box):

Failed to resolve dependency: @mdx-js/react, present in 'optimizeDeps.include'
Failed to resolve dependency: @storybook/addon-docs, present in 'optimizeDeps.include'
Failed to resolve dependency: @storybook/addon-essentials/docs/mdx-react-shim, present in 'optimizeDeps.include'
Failed to resolve dependency: react-dom, present in 'optimizeDeps.include'
Failed to resolve dependency: react-dom/client, present in 'optimizeDeps.include'
Failed to resolve dependency: react, present in 'optimizeDeps.include'
Failed to resolve dependency: react/jsx-runtime, present in 'optimizeDeps.include'
Failed to resolve dependency: react/jsx-runtime, present in 'optimizeDeps.include' (x2)
Failed to resolve dependency: react/jsx-dev-runtime, present in 'optimizeDeps.include'
Failed to resolve dependency: react, present in 'optimizeDeps.include'

so i’m guessing the viteFinal function that i tried to use isn’t working and is breaking things more.

@IanVS
Copy link
Member

IanVS commented Apr 25, 2023

@acusti would you be willing to create a minimal reproduction and open a new issue so we can dig into it more there?

@acusti
Copy link

acusti commented Apr 25, 2023

@IanVS ok, i can do that when i get a free moment

@acusti
Copy link

acusti commented Oct 11, 2023

@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 packageExtensions in my .yarnrc.yml file and use the getAbsolutePath helper in my .storybook/main.ts file. here’s all of the necessary pieces (i’m using v7.5.0-alpha.5 and react-docgen v6.x):

storybook workspace’s package.json:

    "devDependencies": {
        "@babel/core": "^7.20.5",
        "@storybook/addon-docs": "7.5.0-alpha.5",
        "@storybook/addon-essentials": "7.5.0-alpha.5",
        "@storybook/addon-interactions": "7.5.0-alpha.5",
        "@storybook/addon-links": "7.5.0-alpha.5",
        "@storybook/addon-onboarding": "^1.0.8",
        "@storybook/blocks": "7.5.0-alpha.5",
        "@storybook/manager-api": "7.5.0-alpha.5",
        "@storybook/react": "7.5.0-alpha.5",
        "@storybook/react-vite": "7.5.0-alpha.5",
        "@storybook/testing-library": "^0.2.1",
        "@types/react": "^18.0.25",
        "core-js": "^3.26.1",
        "react": "^18.2.0",
        "react-dom": "^18.2.0",
        "storybook": "7.5.0-alpha.5",
        "typescript": "^5.1.6",
        "vite": "^4.4.9"
    },

root .yarnrc.yml:

plugins:
  - path: .yarn/plugins/@yarnpkg/plugin-workspace-tools.cjs
    spec: "@yarnpkg/plugin-workspace-tools"

yarnPath: .yarn/releases/yarn-3.6.3.cjs

packageExtensions:
    'eslint-config-react-app@*':
        dependencies:
            '@babel/plugin-syntax-flow': '^7.18.6'
            '@babel/plugin-transform-react-jsx': '^7.18.6'
    '@storybook/builder-vite@*':
        dependencies:
            '@storybook/addon-docs': '7.5.0-alpha.5'
            'react': '^18.2.0'
            'react-dom': '^18.2.0'
    '@storybook/docs-tools@*':
        dependencies:
            'react': '^18.2.0'
            'react-dom': '^18.2.0'
    '@storybook/core-common@*':
        dependencies:
            '@storybook/react-vite': '7.5.0-alpha.5'
            'react': '^18.2.0'
            'react-dom': '^18.2.0'
            'vite': '^4.4.9'

.storybook/main.ts:

import { dirname, join } from 'path';

import type { StorybookConfig } from '@storybook/react-vite';

/**
 * This function is used to resolve the absolute path of a package.
 * It is needed in projects that use Yarn PnP or are set up within a monorepo.
 */
function getAbsolutePath(value: string): any {
    return dirname(require.resolve(join(value, 'package.json')));
}

const config: StorybookConfig = {
    addons: [
        getAbsolutePath('@storybook/addon-links'),
        getAbsolutePath('@storybook/addon-essentials'),
        getAbsolutePath('@storybook/addon-onboarding'),
        getAbsolutePath('@storybook/addon-interactions'),
    ],
    docs: {
        autodocs: 'tag',
    },
    framework: {
        name: getAbsolutePath('@storybook/react-vite'),
        options: {},
    },
    stories: [
        '../stories/Introduction.stories.mdx',
        '../stories/**/*.stories.mdx',
        '../stories/**/*.stories.@(js|jsx|ts|tsx)',
    ],
    typescript: {
        reactDocgen: 'react-docgen',
    },
};

export default config;

@Pinpickle
Copy link
Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests