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

SB7: Yarn PnP broken in both vite & webpack #20405

Closed
blikblum opened this issue Dec 24, 2022 · 27 comments · Fixed by #21046
Closed

SB7: Yarn PnP broken in both vite & webpack #20405

blikblum opened this issue Dec 24, 2022 · 27 comments · Fixed by #21046

Comments

@blikblum
Copy link

Is your feature request related to a problem? Please describe

In a clean install of storybook v7 with vite-builder webpack@4 dependency is added to the project

https://github.com/blikblum/yarn-storybook/blob/master/yarn.lock#L2792

Describe the solution you'd like

Remove webpack dependency from @storybook/core-common

Describe alternatives you've considered

No response

Are you able to assist to bring the feature to reality?

no

Additional context

No response

@joshwooding
Copy link
Member

@blikblum Hi, your lock file is showing @storybook/core-common@npm:^6.4.3. This is from @storybook/builder-vite@npm:^0.2.6. This shouldn't be needed since you're using @storybook/web-components-vite. If you remove @storybook/builder-vite from you package.json, webpack@4 should be removed.

@blikblum
Copy link
Author

@joshwooding if I dont add it it fails with Error:

Error: Your application tried to access @storybook/builder-vite, but it isn't declared in your dependencies; this makes the require call ambiguous and unsound.
ERR!
ERR! Required package: @storybook/builder-vite (via "@storybook\builder-vite")

See https://github.com/blikblum/yarn-storybook/blob/master/README.md for reproduction steps

@shilman
Copy link
Member

shilman commented Dec 25, 2022

I agree with @joshwooding 's suggestion above. However if you reference @storybook/builder-vite in your code, you need to add it as a dependency in yarn pnp. But if this is the case you should be using @storybook/builder-vite@next which will install the 7.0 prerelease version of the vite builder, which does not have any webpack transitive dependencies

@shilman shilman closed this as completed Dec 25, 2022
@blikblum
Copy link
Author

@shilman I dont use builder-vite. Its a requirement of a clean , minimal install of storybook in an empty project. Look at https://github.com/blikblum/yarn-storybook/blob/master/README.md to see the steps i did and the errors along the way.

Seems theres a bug somewhere getting that using webcomponents-vite should bot need this

This shouldn't be needed since you're using @storybook/web-components-vit

@shilman shilman added bug yarn / npm Yarn / npm acting weird and removed question / support labels Dec 26, 2022
@shilman shilman changed the title [Feature Request]:[v7] remove webpack@4 dependency when using vite-builder SB7: Yarn PnP support broken Dec 26, 2022
@shilman shilman reopened this Dec 26, 2022
@shilman
Copy link
Member

shilman commented Dec 26, 2022

Hi @blikblum, thanks for providing the reproduction steps. I can confirm we have some issues with PnP support.

Running your reproduction above, I also encountered the @storybook/web-components/package.json issue. This looks like it's due to running npx sb@next init --builder=vite in a yarn pnp project. It works if I run yarn dlx sb@next init --builder=vite instead.

After that, it requires the @storybook/builder-vite package as you describe. This means we have some more work to do here. I'll consult with the team and hopefully we can do something here for 7.0. We definitely need to upgrade our testing game for pnp before the release.

As for your workaround of adding @storybook/builder-vite as a dev dep, the problem was that you were adding a 6.x version to a 7.0 project.

This does the trick:

yarn add @storybook/builder-vite@next --dev

Side note: when I try the same thing in a webpack project, I get:

Required package: @babel/core
Required by: babel-loader@virtual:541788efb2ae15be935331df3afcdd12b4e4d1454ba837661f5b0e56ed6f3cf968163727303a83de5631abb0e03ecf977fefd596588a775fc47e066c60e0e6f9#npm:8.3.0 (via /Users/shilman/projects/testing/storybook-yarn/.yarn/__virtual__/babel-loader-virtual-80fa1a04bc/0/cache/babel-loader-npm-8.3.0-a5239d7ed2-d48bcf9e03.zip/node_modules/babel-loader/lib/)

Ancestor breaking the chain: @storybook/preset-web-components-webpack@virtual:90083f79b5743cd6d299b83eaf48725ad66fc94cee9ff01922972141632d8aa5293bdc8d4acbff105dea6efbbed1d158ccd1e71d72b0d593d1da77f6c994a78c#npm:7.0.0-beta.15

So it looks like we have some work to do there too.

cc @ndelangen @IanVS

@shilman shilman moved this to Required for RC in Core Team Projects Dec 26, 2022
@shilman shilman changed the title SB7: Yarn PnP support broken SB7: Yarn PnP support broken in vite & webpack Dec 26, 2022
@shilman shilman changed the title SB7: Yarn PnP support broken in vite & webpack SB7: Yarn PnP support broken in both vite & webpack Dec 26, 2022
@shilman shilman changed the title SB7: Yarn PnP support broken in both vite & webpack SB7: Yarn PnP broken in both vite & webpack Dec 26, 2022
@blikblum
Copy link
Author

#20404 is the original issue for the PnP crash. This was a "side quest". You may want to mark one or other as duplicate

@vplanet-aiden
Copy link

any progress?

@IanVS
Copy link
Member

IanVS commented Jan 10, 2023

@vplanet-aiden it's in the 7.0 Burndown project and listed as a blocker for a Release Candidate, so the team will be sure to address it soon. As I understand it, the next step is to upgrade verdaccio, so that we can start testing with yarn pnp in our CI. If you'd like to attempt an upgrade and post a PR, that would be a big help!

@shilman
Copy link
Member

shilman commented Jan 10, 2023

Son of a gun!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.0-beta.24 containing PR #20561 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb upgrade --prerelease

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

@shilman shilman closed this as completed Jan 10, 2023
@github-project-automation github-project-automation bot moved this from Required for RC to Done in Core Team Projects Jan 10, 2023
@IanVS IanVS reopened this Jan 10, 2023
@ndelangen ndelangen moved this from Done to Required for RC in Core Team Projects Jan 13, 2023
@shilman shilman moved this from Required for RC to Required for GA in Core Team Projects Jan 23, 2023
@shilman shilman moved this from Required for GA to Required for RC in Core Team Projects Feb 2, 2023
@deokseong-kim-toss
Copy link

To me, yarn pnp + vite (yarn dlx sb@next init --builder=vite) makes following error

WARN   Failed to load preset: "@storybook/react-vite/preset"
ERR! 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.
ERR! 
ERR! Required package: @storybook/react-vite (via "@storybook/react-vite/preset")
ERR! Required by: @storybook/core-common@npm:7.0.0-beta.43

and packageExtensions couldn't fix it

packageExtensions:
  "@storybook/core-common@^7.0.0-beta.43":
    dependencies:
      "@storybook/react-vite": "^7.0.0-beta.43"

@ndelangen
Copy link
Member

@deokseong-kim-toss can you share you main.js file?

I think a few require.resolve() wrappings might be the solution for you.

@ndelangen
Copy link
Member

@shilman we've discussed this a few times before. I think this ticket is also about us getting packages/dependencies in order (which I think we've pretty much completed?), but it's also conflated with the problem listen below, we should discuss.

With the way we’ve implemented @storybook/core-common and how main.ts is structured, there’s not much further we can do right now for users who opted to init storybook without the --pnp CLI-flag.

We’ve fixed a lot of peerDependencies issues we had, most importantly we have react as a peerDependency in all places now. We also made fixes in regard of typescript being a peerDependency or become optional.

But what remains is that we tell users that their main.ts file should reference modules as a string. And then later storybook’s internals are responsible for loading that module.
That is just fundamentally incompatible with pnp -mode.

Users might run into an error like this:

WARN   Failed to load preset: "@storybook/react-vite/preset"
ERR! 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.
ERR! 
ERR! Required package: @storybook/react-vite (via "@storybook/react-vite/preset")
ERR! Required by: @storybook/core-common@npm:7.0.0-beta.43

What is really happening here is that the storybook core is trying to require a module, which it has no dependency on.

It cannot have a dependency on it, because then it’d have to have a dependency on every possible builder, every possible framework, every possible preset, every possible addon. This is not feasible.

The solution would be to change how storybook’s main.ts works.. We’ve seen this in many instances now in the ecosystem:

import { defineConfig } from 'xyz';
import plugin1 from 'xyz-plugin-1';
import plugin2 from 'xyz-plugin-2';

export default defineConfig({
  plugins: [plugin1(), plugin2()],
};

This pattern is very versatile, and very robust. Because the imports happen in the root of the project, that’s where the node resolution mechanism will search for the modules. (which is correct).

But this is how storybook’s main.ts looks:

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

export default {
	addons: ['@storybook/addon-1', '@storybook/addon-2'],
} satisfies StorybookConfig;

This will result into problems when the users is using pnp.

We can ask the user to change their main.ts file:

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

export default {
	addons: [
		path.dirname(require.resolve('@storybook/addon-1/package.json')),
		path.dirname(require.resolve('@storybook/addon-2/package.json'))
	],
} satisfies StorybookConfig;

This will ensure that when storybook tries to require the given input, it will already be an absolute path on the file-system; thus not involve the node module resolution system.

@ndelangen ndelangen moved this from Required for RC to In Progress in Core Team Projects Feb 7, 2023
@deokseong-kim-toss
Copy link

deokseong-kim-toss commented Feb 7, 2023

@ndelangen Sure.

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

const config: StorybookConfig = {
  "stories": [
    "../src/**/*.mdx",
    "../src/**/*.stories.@(js|jsx|ts|tsx)"
  ],
  "addons": [
    "@storybook/addon-links",
    "@storybook/addon-essentials",
    "@storybook/addon-interactions"
  ],
  "framework": {
    "name": "@storybook/react-vite",
    "options": {}
  },
  "docs": {
    "autodocs": "tag"
  }
};
export default config;

nothing is changed since sb init.

It also doesn't works without all addons.

@millsb
Copy link

millsb commented Feb 7, 2023

Thanks for the help. I understand PNP does not always make things easy. I am experiencing the same issue as deokseong-kim-toss. It was mentioned that there exists a --use-pnp CLI flag. If I used that to initialize the project:

yarn dlx sb@next init builder=vite --use-pnp`

My main.ts looks like:

/** @type { import('%%path.dirname(require.resolve(path.join('@storybook/react-vite', 'package.json')))%%').StorybookConfig } */
const config = {
  "stories": [
    "../src/**/*.mdx",
    "../src/**/*.stories.@(js|jsx|ts|tsx)"
  ],
  "addons": [
    "%%path.dirname(require.resolve(path.join('@storybook/addon-links', 'package.json')))%%",
    "%%path.dirname(require.resolve(path.join('@storybook/addon-essentials', 'package.json')))%%",
    "%%path.dirname(require.resolve(path.join('@storybook/addon-interactions', 'package.json')))%%"
  ],
  "framework": {
    "name": "%%path.dirname(require.resolve(path.join('@storybook/react-vite', 'package.json')))%%",
    "options": {}
  },
  "docs": {
    "autodocs": "tag"
  }
};
export default config;

It's unclear to me what the %% is trying to do (versus the example above). However, just running with that nets me this error:

 Error: @storybook/core-common tried to access %path.dirname(require.resolve(path.join('@storybook, but it isn't declared in its dependencies; this makes the require call ambiguous and unsound.

If I remove the string quotes and %% from the path.dirname() statements to make the paths resolve immediately, it seems to be able to resolve the addons and framework imports. But I just end up with the same type of PNP error we've been experiencing, but this time with modules not part of main.ts.

Error: @storybook/core-server tried to access @storybook/manager, but it isn't declared in its dependencies; this makes the require call ambiguous and unsound.

@ndelangen
Copy link
Member

ndelangen commented Feb 7, 2023

@millsb ouch, that looks like it's close but also very wrong!

It should be:

/** @type { import('@storybook/react-vite').StorybookConfig } */
const config = {
  "stories": [
    "../src/**/*.mdx",
    "../src/**/*.stories.@(js|jsx|ts|tsx)"
  ],
  "addons": [
    path.dirname(require.resolve(path.join('@storybook/addon-links', 'package.json'))),
    path.dirname(require.resolve(path.join('@storybook/addon-essentials', 'package.json'))),
    path.dirname(require.resolve(path.join('@storybook/addon-interactions', 'package.json'))),
  ],
  "framework": {
    "name": path.dirname(require.resolve(path.join('@storybook/react-vite', 'package.json'))),
    "options": {}
  },
  "docs": {
    "autodocs": "tag"
  }
};
export default config;

☝️ can you try similar adjustments to your main.js file, @deokseong-kim-toss ?

@millsb
Copy link

millsb commented Feb 7, 2023

Thanks. Same issue as before. Looks like its an issue with modules outside of main.ts now.

Error: @storybook/core-server tried to access @storybook/manager, but it isn't declared in its dependencies; this makes the require call ambiguous and unsound.

Required package: @storybook/manager (via "@storybook/manager/package.json")
Required by: @storybook/core-server@npm:7.0.0-beta.43 (via /Users/bryan/cz/vite-project/.yarn/cache/@storybook-core-server-npm-7.0.0-beta.43-3a4959bd8d-5da360b62a.zip/node_modules/@storybook/core-server/dist/presets/)

(Can confirm @storybook/manager@7.0.0-beta.43 exists in yarn.lock)

@ndelangen
Copy link
Member

@millsb that specific problem should be fixed by #20989

@deokseong-kim-toss
Copy link

deokseong-kim-toss commented Feb 7, 2023

Thanks @ndelangen. Same result as @millsb, so i tried packageExtensions as if that pr is merged

packageExtensions:
  "@storybook/core-server@^7.0.0-beta.43":
    dependencies:
      "@storybook/manager": "^7.0.0-beta.43"

and builder works! but another error comes up in the browser.

The requested module '/@fs/Users/deokseong.kim/Works/Toss/playgrounds/monorepo-test/.yarn/cache/jest-mock-npm-27.5.1-22d1da854d-f5b5904bb1.zip/node_modules/jest-mock/build/index.js?v=6f910e24' does not provide an export named 'ModuleMocker'

스크린샷 2023-02-08 오전 12 14 26

@IanVS
Copy link
Member

IanVS commented Feb 7, 2023

@deokseong-kim-toss glad you could confirm that the builder is working! I'd suggest hopping into our Discord to get help with that error (discord.gg/storybook), but at first glance, it looks like maybe you're using jest-mock instead of @storybook/jest. Let's keep this issue focused about yarn pnp support.

@deokseong-kim-toss
Copy link

Thanks! I'll continue in discord chat. However repo has no other dependencies like jest-mock and it seems to @storybook/addon-interactions problem which has jest mock dependencies

스크린샷 2023-02-08 오전 12 33 45

@ndelangen
Copy link
Member

I just discovered a whole lot, trying to setup a yarn pnp sandbox, so we can finally do proper e2e regression tests on this.

The storybook CLI tries to find out where some packages are installed, to be able to copy files out (demo files) and do some other stuff.

This simply does . not . work . with pnp enabled. It can't work.

But I found a proper way to determine where the package IS installed, using the pnp file yarn generates!

require('./.pnp.cjs').resolveRequest('@storybook/react/package.json', './');

...will output the virtual location of the file if it was requested from the root. 🎉

@IanVS
Copy link
Member

IanVS commented Feb 8, 2023

That's awesome. If you're able to set up a pnp sandbox, do you think it would be much more work to create a pnpm sandbox as well?

@ndelangen
Copy link
Member

yes possibly, but I'd need a generator that outputs a pnpm project.

@IanVS
Copy link
Member

IanVS commented Feb 9, 2023

pnpm create vite can be used. Though it doesn't automatically install dependencies or create the lockfile, so --package-manager pnpm would need to be used with the SB init. I'll try to find a generator that creates the lockfile.

@ndelangen
Copy link
Member

@IanVS I'd love to 🍐 with you on this, or discuss what we could do to resolve this:

ERR! Error: [vite]: Rollup failed to resolve import "/tmp/storybook/sandbox/html-vite-default-js/node_modules/@storybook/addon-essentials/docs/preview" from "/virtual:/@storybook/builder-vite/vite-app.js".
ERR! This is most likely unintended because it can break your application at runtime.

@ndelangen
Copy link
Member

ndelangen commented Feb 10, 2023

I can reproduce this problem now:

📐 reading version of storybook
🚛 listing storybook packages
🎬 starting verdaccio (this takes ±5 seconds, so be patient)
(node:59594) Warning: deprecate: multiple logger configuration is deprecated, please check the migration guide.
(Use `node --trace-warnings ...` to show where the warning was created)
🟢 install > 🟢 compile > ✅ publish > 🔊 run-registry > 🔄 generate > 🟡 sandbox

🤹‍♂️ Generating sandboxes with a concurrency of 9
🧬 generating PNP (react-webpack/18-ts)
🌿 verdaccio running on http://localhost:6001
👤 add temp user to verdaccio
 error--- authenticating for user user failed. Error: bad username/password, access denied
(node:59594) [DEP0106] DeprecationWarning: crypto.createDecipher is deprecated.
 error--- authenticating for user user failed. Error: bad username/password, access denied
📦 found 90 storybook packages at version 7.0.0-beta.46
📦 Configuring local registry: http://localhost:6001
🎁 Installing storybook
📦 Restoring registry: https://registry.yarnpkg.com/
Error running task generate:
Error: Command failed with exit code 1: /Users/me/Projects/Storybook/core/code/lib/cli/bin/index.js init --yes

     Error: Cannot find module '@storybook/react/package.json'
Require stack:
- /Users/me/Projects/Storybook/core/code/lib/cli/dist/generate.js
- /Users/me/Projects/Storybook/core/code/lib/cli/bin/index.js
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:956:15)
    at Function.resolve (node:internal/modules/cjs/helpers:108:19)
    at getRendererDir (/Users/me/Projects/Storybook/core/code/lib/cli/dist/generate.js:1:7147)
    at componentsPath (/Users/me/Projects/Storybook/core/code/lib/cli/dist/generate.js:11:1474)
    at copyComponents (/Users/me/Projects/Storybook/core/code/lib/cli/dist/generate.js:11:2661)
    at async baseGenerator (/Users/me/Projects/Storybook/core/code/lib/cli/dist/generate.js:40:738)
    at async generator6 (/Users/me/Projects/Storybook/core/code/lib/cli/dist/generate.js:61:448)
    at async doInitiate (/Users/me/Projects/Storybook/core/code/lib/cli/dist/generate.js:381:78)
    at async withTelemetry (/Users/me/Projects/Storybook/core/code/lib/core-server/dist/index.js:33:5568)
    at async initiate (/Users/me/Projects/Storybook/core/code/lib/cli/dist/generate.js:387:133)
    at makeError (file:///Users/me/Projects/Storybook/core/scripts/node_modules/execa/lib/error.js:59:11)
    at handlePromise (file:///Users/me/Projects/Storybook/core/scripts/node_modules/execa/index.js:119:26)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async execaCommand (/Users/me/Projects/Storybook/core/scripts/utils/exec.ts:29:10)
    at async sbInit (/Users/me/Projects/Storybook/core/scripts/sandbox-generators/generate-sandboxes.ts:35:3)
    at async /Users/me/Projects/Storybook/core/scripts/sandbox-generators/generate-sandboxes.ts:84:7
    at async withLocalRegistry (/Users/me/Projects/Storybook/core/scripts/sandbox-generators/generate-sandboxes.ts:45:5)
    at async addStorybook (/Users/me/Projects/Storybook/core/scripts/sandbox-generators/generate-sandboxes.ts:81:5)
    at async /Users/me/Projects/Storybook/core/scripts/sandbox-generators/generate-sandboxes.ts:177:9 {
  shortMessage: 'Command failed with exit code 1: /Users/me/Projects/Storybook/core/code/lib/cli/bin/index.js init --yes',
  command: '/Users/me/Projects/Storybook/core/code/lib/cli/bin/index.js init --yes',
  escapedCommand: '"/Users/me/Projects/Storybook/core/code/lib/cli/bin/index.js" init --yes',
  exitCode: 1,
  signal: undefined,
  signalDescription: undefined,
  stdout: undefined,
  stderr: '\n' +
    "     Error: Cannot find module '@storybook/react/package.json'\n" +
    'Require stack:\n' +
    '- /Users/me/Projects/Storybook/core/code/lib/cli/dist/generate.js\n' +
    '- /Users/me/Projects/Storybook/core/code/lib/cli/bin/index.js\n' +
    '    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:956:15)\n' +
    '    at Function.resolve (node:internal/modules/cjs/helpers:108:19)\n' +
    '    at getRendererDir (/Users/me/Projects/Storybook/core/code/lib/cli/dist/generate.js:1:7147)\n' +
    '    at componentsPath (/Users/me/Projects/Storybook/core/code/lib/cli/dist/generate.js:11:1474)\n' +
    '    at copyComponents (/Users/me/Projects/Storybook/core/code/lib/cli/dist/generate.js:11:2661)\n' +
    '    at async baseGenerator (/Users/me/Projects/Storybook/core/code/lib/cli/dist/generate.js:40:738)\n' +
    '    at async generator6 (/Users/me/Projects/Storybook/core/code/lib/cli/dist/generate.js:61:448)\n' +
    '    at async doInitiate (/Users/me/Projects/Storybook/core/code/lib/cli/dist/generate.js:381:78)\n' +
    '    at async withTelemetry (/Users/me/Projects/Storybook/core/code/lib/core-server/dist/index.js:33:5568)\n' +
    '    at async initiate (/Users/me/Projects/Storybook/core/code/lib/cli/dist/generate.js:387:133)',
  failed: true,
  timedOut: false,
  isCanceled: false,
  killed: false
}
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
(norbert/pnp-experiment-2) [1] %      

I first tried to apply a general fix here:
#20998

Now I'm trying a less risky approach here:
#21046

@shilman
Copy link
Member

shilman commented Feb 15, 2023

Son of a gun!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.0-beta.48 containing PR #21046 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb@next upgrade --prerelease

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