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: Upgrading to Storybook 8.3.0 prereleases causes "Can't resolve 'tty' in..." error when compiling the manager #76

Closed
JReinhold opened this issue Aug 29, 2024 · 11 comments · Fixed by #80

Comments

@JReinhold
Copy link

JReinhold commented Aug 29, 2024

Steps to reproduce

  1. Download the sandbox from this repo here: https://github.com/rspack-contrib/storybook-rsbuild/tree/main/sandboxes/react-18
  2. optionally replace the workspace:* dependency in package.json with * if you like me, just downloaded that specific directory.
  3. Delete the pnpm lock-file.
  4. pnpm install
  5. Upgrade Storybook with npx storybook@next upgrade
  6. Start Storybook with pnpm storybook
  7. See error in the terminal:
start   Compiling...
error   Compile error:
Failed to compile, check the errors for troubleshooting.
File: /Users/jeppe/dev/temp/rspack-contrib storybook-rsbuild main sandboxes-react-18/node_modules/.pnpm/@[email protected][email protected]/node_modules/@storybook/instrumenter/dist/index.js:1:1
  × Module not found: Can't resolve 'tty' in '/Users/jeppe/dev/temp/rspack-contrib storybook-rsbuild main sandboxes-react-18/node_modules/.pnpm/@[email protected][email protected]/node_modules/@storybook/instrumenter/dist'
   ╭─[1:2542]
 1 │ "use strict";var __defProp=Obje...
...
   ╰────

Tip: "tty" is a built-in Node.js module and cannot be imported in client-side code.
Check if you need to import Node.js module. If needed, you can use "@rsbuild/plugin-node-polyfill".

... And as a UI overlay in the browser :

File: /Users/jeppe/dev/temp/rspack-contrib storybook-rsbuild main sandboxes-react-18/node_modules/.pnpm/@[email protected][email protected]/node_modules/@storybook/instrumenter/dist/index.js:1:1
  × Module not found: Can't resolve 'tty' in '/Users/jeppe/dev/temp/rspack-contrib storybook-rsbuild main sandboxes-react-18/node_modules/.pnpm/@[email protected][email protected]/node_modules/@storybook/instrumenter/dist'
   ╭─[1:2542]
 1 │ "use strict";var __defProp=Obje...

Workaround

As the tip in the terminal output suggests, adding the Node polyfill plugin resolves the issue:

// rsbuild.config.ts

import { defineConfig } from '@rsbuild/core'
import { pluginReact } from '@rsbuild/plugin-react'
import { pluginNodePolyfill } from '@rsbuild/plugin-node-polyfill';
export default defineConfig({
  plugins: [pluginReact(), pluginNodePolyfill()],
})

Additional info

Node polyfills shouldn't be required for this. It seems that both from the filename File: .../node_modules/@storybook/instrumenter/dist/index.js:1:1 and from the file content, RSBuild is resolving CJS / Node exports instead of the ESM exports. I don't know enough about RSBuild to understand if this is on purpose or not, but given it's bundling for the browser, I'd expect it to use the import conditions of packages' export maps and not node or require.

The offending chain of dependencies is:

  1. @storybook/instrumenter imports processError from @vitest/utils/error https://github.com/storybookjs/storybook/blob/next/code/lib/instrumenter/src/instrumenter.ts/#L14-L15
  2. @vitest/utils/error imports format from @vitest/pretty-format https://github.com/vitest-dev/vitest/blob/122e4fe0a7d4f208205617069b3df6a0ffaf7467/packages/utils/src/diff/index.ts#L11-L14
  3. @vitest/pretty-format imports default from tinyrainbow https://github.com/vitest-dev/vitest/blob/122e4fe0a7d4f208205617069b3df6a0ffaf7467/packages/pretty-format/src/index.ts#L8
  4. The node condition of tinyraindow imports node:tty https://github.com/tinylibs/tinyrainbow/blob/master/src/node.ts#L1

Ideally the /browser.js file of tinyraindbow should be hit via the import condition instead of the /node.js file.

My guess is that this is caused by Storybook internally upgrading to Vitest v2.0 in storybookjs/storybook#28788, which introduced Vitest utils' change from picocolor to tinyrainbow in vitest-dev/vitest#6077

@JReinhold JReinhold changed the title Upgrading to Storybook 8.3.0 prereleases causes "Can't resolve 'tty' in..." error when compiling stories Bug: Upgrading to Storybook 8.3.0 prereleases causes "Can't resolve 'tty' in..." error when compiling stories Aug 29, 2024
@JReinhold JReinhold changed the title Bug: Upgrading to Storybook 8.3.0 prereleases causes "Can't resolve 'tty' in..." error when compiling stories Bug: Upgrading to Storybook 8.3.0 prereleases causes "Can't resolve 'tty' in..." error when compiling the manager Aug 29, 2024
@jeffijoe
Copy link

Upgrading to Storybook 8.3.0-beta.1 gives me a different error:

TypeError: rsbuildServer.connectWebSocket is not a function
    at Module.start (./node_modules/.pnpm/[email protected]_@[email protected]_@[email protected]_@swc+h_obbwq5khpv2gzzd3qhfa54feza/node_modules/storybook-builder-rsbuild/dist/index.js:690:17)
    at async storybookDevServer (./node_modules/.pnpm/@[email protected]/node_modules/@storybook/core/dist/core-server/index.cjs:47422:11)
    at async buildOrThrow (./node_modules/.pnpm/@[email protected]/node_modules/@storybook/core/dist/core-server/index.cjs:46675:12)
    at async buildDevStandalone (./node_modules/.pnpm/@[email protected]/node_modules/@storybook/core/dist/core-server/index.cjs:48612:78)
    at async withTelemetry (./node_modules/.pnpm/@[email protected]/node_modules/@storybook/core/dist/core-server/index.cjs:47174:12)
    at async dev (./node_modules/.pnpm/@[email protected]/node_modules/@storybook/core/dist/cli/bin/index.cjs:2877:3)
    at async r.<anonymous> (./node_modules/.pnpm/@[email protected]/node_modules/@storybook/core/dist/cli/bin/index.cjs:2929:74)

Not sure if I should open another issue since this is also related to the upgrade. On the latest version of storybook-rsbuild.

@fi3ework
Copy link
Member

fi3ework commented Sep 2, 2024

@jeffijoe You need to bump your Rsbuild version, as per 219d0d5.

@fi3ework
Copy link
Member

fi3ework commented Sep 2, 2024

I'm investing this issue, Rsbuild could handle exports, browser fields correctly (I checked it again with a minimal demo and confirmed that). It might broke somewhere else when integrating.

@jeffijoe
Copy link

jeffijoe commented Sep 2, 2024

npm-check didn’t notify me of an update to Rsbuild, I’ll try installing latest tomorrow

@fi3ework
Copy link
Member

fi3ework commented Sep 2, 2024

@JReinhold, cc @shilman

First, thank you for providing such detailed error logs, I have found the reason for the problem:

  1. The whole thing originated from here: https://github.com/guillaumewuip/storybook-rsbuild/blob/main/sandboxes/react-18/.storybook/main.ts#L19, I used the getAbsolutePath method, which is commonly used in Storybook, to resolve the package location.
  2. The first step's behavior caused that in https://github.com/storybookjs/storybook/blob/00c1524e0e393da445b3a5e2d82197d453907dcc/code/core/src/common/presets.ts#L135, dist/preview.mjs was not resolved as a result, but got preview.js instead, which is a CJS module, which means all dependencies couldn’t be parsed from the exports.import field thereafter.

The final stack causing the problem was at https://github.com/storybookjs/storybook/blob/next/code/core/src/common/utils/safeResolve.ts/#L13. I created a stackblitz (https://stackblitz.com/edit/node-fg7rkr?file=index.js) for you to clearly understand the difference in interpretation of /preview in case with and without getAbsolutePath.

When getAbsolutePath is used, require.resolve is interpreted as a package and the result is as expected; When without, it is interpreted as an absolute path module. You can try commenting out the line https://github.com/rspack-contrib/storybook-rsbuild/blob/main/sandboxes/react-18/.storybook/main.ts#L19 where I used getAbsolutePath and everything will be fine.

I think getAbsolutePath will still be widely used, this problem may need better handling within Storybook IMO. I'm glad to help if needed.

@JReinhold
Copy link
Author

Hmm that's interesting @fi3ework, I've seen something similar before.

But just to be clear, are you saying you think this breaks for any Storybook users using getAbsolutePath(), or just with rsbuild?

I've just tested with a Vite-based project that also uses uses getAbsolutePath(), and it loads dist/preview.mjs directly (skipping /preview.js entirely)

@fi3ework
Copy link
Member

fi3ework commented Sep 3, 2024

Comparing webpack builder, I found it will still go into the preview.js module and then resolved to ./dist/preview.mjs instead of ./dist/preview.js which keeps the correct behaviour. The difference comes from resolve.extensions in webpack builder (https://github.com/storybookjs/storybook/blob/00c1524e0e393da445b3a5e2d82197d453907dcc/code/builders/builder-webpack5/src/preview/iframe-webpack.config.ts#L237) and rsbuild builder (https://github.com/web-infra-dev/rsbuild/blob/8c0f253ed7dad644193e2fede27f495fb4126396/packages/core/src/plugins/resolve.ts#L30-L35). I think Storybook don't need to do nothing here, and we can fix this by align the config with Rsbuild builder. Thanks for helping debugging!

The reason we haven't aligned this behavior before is that Rsbuild's configuration is also highly optimized. @chenjiahan, do you think we can rearrange the ⁠resolve.extensions order to give ESM extensions higher priority? This might happens in other dual packages.

image

@chenjiahan
Copy link
Member

It's ok, we can give .mjs a higher prority.

  const extensions = [
    // most projects are using TypeScript, resolve .ts(x) files first to reduce resolve time.
    '.ts',
    '.tsx',
+   '.mjs',
    '.js',
    '.jsx',
-   '.mjs',
    '.json',
  ];

@fi3ework
Copy link
Member

fi3ework commented Sep 3, 2024

Thank you for Storybook team's reaching out! The fix will be released soon and be compatible with Storybook 8.3.0.

@JReinhold
Copy link
Author

Fantastic team work!

@jeffijoe
Copy link

jeffijoe commented Sep 3, 2024

Just confirming that with the latest beta of both Storybook and Rsbuild everything works! Great work everyone! 🙌

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

Successfully merging a pull request may close this issue.

4 participants