[Bug]: Vite CJS warning still present in v8 RC

dan-mba commented 6 months ago

Describe the bug

Using Vite 5 with Storybook 8 RC produces the following warning:

The CJS build of Vite's Node API is deprecated. See https://vitejs.dev/guide/troubleshooting.html#vite-cjs-node-api-deprecated for more details

According to #24333 this was supposed to be fixed with v8. As a RC has been released, I would assume this should have been fixed by now.

To Reproduce

https://github.com/dan-mba/react-svg-components/tree/sb-8

System

Storybook Environment Info:

  System:
    OS: Linux 5.15 Ubuntu 22.04.4 LTS 22.04.4 LTS (Jammy Jellyfish)
    CPU: (16) x64 AMD Ryzen 7 5700G with Radeon Graphics
    Shell: 5.1.16 - /bin/bash
  Binaries:
    Node: 20.11.1 - ~/.nvm/versions/node/v20.11.1/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v20.11.1/bin/yarn
    npm: 10.4.0 - ~/.nvm/versions/node/v20.11.1/bin/npm <----- active
    pnpm: 8.7.0 - ~/.nvm/versions/node/v20.11.1/bin/pnpm
  Browsers:
    Chrome: 122.0.6261.94
  npmPackages:
    @storybook/addon-essentials: ^8.0.0-rc.1 => 8.0.0-rc.1
    @storybook/react: ^8.0.0-rc.1 => 8.0.0-rc.1
    @storybook/react-vite: ^8.0.0-rc.1 => 8.0.0-rc.1
    eslint-plugin-storybook: ^0.8.0 => 0.8.0
    storybook: ^8.0.0-rc.1 => 8.0.0-rc.1

Additional context

No response

codeart1st commented 6 months ago

I also tried 8.0.0-rc.1 for our own setup. Can't confirm this, but have you tried adding this "type": "module" to your package.json?

Staremang commented 6 months ago

I also tried 8.0.0-rc.1 for our own setup. Can't confirm this, but have you tried adding this "type": "module" to your package.json?

Confirmed. I have the same error

vanessayuenn commented 6 months ago

@JReinhold @IanVS any idea why #24395 didn't fully solve this?

IanVS commented 6 months ago

Yes, it's because of the way we load the user's config file as CJS (using https://github.com/egoist/esbuild-register), which causes Vite to still show the warning.

JReinhold commented 6 months ago

If users are importing from Vite in their main config like @dan-mba is in their reproduction, they need to use a dynamic import instead of a top-level import to silence this warning, like this:

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

const config: StorybookConfig = {
  stories: ["../src/**/*.stories.@(js|jsx|mjs|ts|tsx)"],
  addons: [ "@storybook/addon-essentials" ],
  framework: {
    name: "@storybook/react-vite",
    options: {},
  },
  docs: {
    autodocs: false,
  },
  async viteFinal(config, {
    configType
  }) {
+    const { mergeConfig } = await import('vite');
    // return the customized config
    return mergeConfig(config, {
      // customize the Vite config here
      base: './'
    });
  },
};
export default config;

This is not trivial to fix for us internally as it requires that we make changes to our preset loading, something @ndelangen is hesitant about and definitely won't happen for 8.0 GA. I think @ndelangen's hope is to do this as part of the ESM transition?

We could perhaps document this dynamic import requirement though? @kylegach

dan-mba commented 6 months ago

If users are importing from Vite in their main config like @dan-mba is in their reproduction, they need to use a dynamic import instead of a top-level import to silence this warning, like this:
import type { StorybookConfig } from "@storybook/react-vite";
- import { mergeConfig } from 'vite';

const config: StorybookConfig = {
  stories: ["../src/**/*.stories.@(js|jsx|mjs|ts|tsx)"],
  addons: [ "@storybook/addon-essentials" ],
  framework: {
    name: "@storybook/react-vite",
    options: {},
  },
  docs: {
    autodocs: false,
  },
  async viteFinal(config, {
    configType
  }) {
+    const { mergeConfig } = await import('vite');
    // return the customized config
    return mergeConfig(config, {
      // customize the Vite config here
      base: './'
    });
  },
};
export default config;
This is not trivial to fix for us internally as it requires that we make changes to our preset loading, something @ndelangen is hesitant about and definitely won't happen for 8.0 GA. I think @ndelangen's hope is to do this as part of the ESM transition?

We could perhaps document this dynamic import requirement though? @kylegach

@JReinhold The dynamic import got rid of the warning for me.

The sample main.js|ts in the Vite Configuration Docs will cause the same warning. Adding a mention about dynamic import here would be useful.

It might also be nice to have a warning when using the update migration cli.

JReinhold commented 6 months ago

The sample main.js|ts in the Vite Configuration Docs will cause the same warning. Adding a mention about dynamic import here would be useful.

Good catch, I've updated the snippets to dynamically import in https://github.com/storybookjs/storybook/pull/26330

It might also be nice to have a warning when using the update migration cli.

This is actually unrelated to upgrading to any Storybook version, it's about users upgrading to Vite 5. So I don't think it's appropriate to show in the upgrade CLI flow.

wuifdesign commented 5 months ago

i still get the warning when using:

typescript: {
    reactDocgen: "react-docgen-typescript",
},

aimad-majdou commented 4 months ago

If users are importing from Vite in their main config like @dan-mba is in their reproduction, they need to use a dynamic import instead of a top-level import to silence this warning, like this:
import type { StorybookConfig } from "@storybook/react-vite";
- import { mergeConfig } from 'vite';

const config: StorybookConfig = {
  stories: ["../src/**/*.stories.@(js|jsx|mjs|ts|tsx)"],
  addons: [ "@storybook/addon-essentials" ],
  framework: {
    name: "@storybook/react-vite",
    options: {},
  },
  docs: {
    autodocs: false,
  },
  async viteFinal(config, {
    configType
  }) {
+    const { mergeConfig } = await import('vite');
    // return the customized config
    return mergeConfig(config, {
      // customize the Vite config here
      base: './'
    });
  },
};
export default config;
This is not trivial to fix for us internally as it requires that we make changes to our preset loading, something @ndelangen is hesitant about and definitely won't happen for 8.0 GA. I think @ndelangen's hope is to do this as part of the ESM transition?

We could perhaps document this dynamic import requirement though? @kylegach

We have changed our main.ts accordingly, but we still get the same error, here is how the file looks:

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',
    '@storybook/addon-coverage',
    '@storybook/addon-themes',
    '@storybook/addon-a11y',
  ],
  framework: {
    name: '@storybook/react-vite',
    options: {},
  },
  docs: {
    autodocs: true,
  },
  async viteFinal(config) {
    const { mergeConfig } = await import('vite');
    const viteTsconfig = await import('vite-tsconfig-paths');
    const tsconfigPaths = viteTsconfig.default;
    return mergeConfig(config, {
      plugins: [tsconfigPaths()], // resolve imports using TypeScript's path mapping
    });
  },
  typescript: {
    check: false,
  },
  core: {
    disableTelemetry: true, // Disables telemetry
  },
};
export default config;

aimad-majdou commented 4 months ago

Upon tracing the origin of the CJS warning, I identified its source in the file: node_modules/vite-plugin-istanbul/dist/index.cjs:

It's worth noting that upon the comment of @wuifdesign, I also removed reactDocgen: "react-docgen-typescript", as it was another trigger for the warning message. As evidenced in this screenshot, the warning comes from node_modules\@joshwooding\vite-plugin-react-docgen-typescript\dist\index.cjs:3:

The warning ceased to appear after the removal of both @storybook/addon-coverage and react-docgen-typescript.

Both files share the same import statement: const vite = require('vite');

This import seems to be the culprit behind the warning. Based on my understanding from https://vitejs.dev/guide/troubleshooting.html#vite-cjs-node-api-deprecated, it appears that vite should be imported dynamically.

Update:

Upon examination of the vite-plugin-react-docgen-typescript, it appears that the issue has been addressed in version 0.3.1, as evidenced by the changes documented here.

In our project, we resolved this by explicitly setting the version in the package.json file:

"overrides": {
    "@joshwooding/vite-plugin-react-docgen-typescript": "0.3.1"
  }

However, it's worth noting that the issue persists in the vite-plugin-istanbul project. They continue to utilize the normal import for vite, as illustrated here.

I have opened a ticket for that: https://github.com/iFaxity/vite-plugin-istanbul/issues/209

vanessayuenn commented 4 months ago

Looks like this issue has been resolved upstream, so closing this now. Let me know if this isn't the case!

zuk-michal commented 4 months ago

@vanessayuenn It's not solved yet for this case:

typescript: {
    reactDocgen: "react-docgen-typescript",
},

greptile-apps[bot] commented 1 month ago

Disclaimer This information might be inaccurate, due to it being generated automatically To address the Vite CJS warning in Storybook 8 RC, update the Vite configuration to use the ESM build of Vite's Node API. Modify the vite.config.ts file in the /code/builders/builder-vite/src directory as follows:

import { defineConfig } from 'vite';

export default defineConfig({
  // existing configuration
  optimizeDeps: {
    esbuildOptions: {
      target: 'esnext',
      format: 'esm',
    },
  },
});

Ensure that the vite dependency in package.json is updated to a version that supports ESM:

"vite": "^5.0.0"

References

#### About Greptile

This response provides a starting point for your research, not a precise solution. Help us improve! Please leave a 👍 if this is helpful and 👎 if it is irrelevant. [Ask Greptile](https://app.greptile.com/chat/github/storybookjs/storybook/next) · [Edit Issue Bot Settings](https://app.greptile.com/apps/github)

kaiyoma commented 1 month ago

I am also seeing this warning, even though my entire repo is ESM (and has been so for over a year). Unlike @zuk-michal I'm disabling doc generation, but changing that setting doesn't seem to matter (I still see the warnings). Our repo is way too large (and proprietary) to provide as a repro, but I'm happy to post config files or logs.

kaiyoma commented 1 month ago

Will anyone be looking into this soon? To be clear, it's not the RC that has this issue; even the latest version of Storybook (8.2.5) exhibits this. And the issue exists even if doc generation is turned off.

I'm asking because this warning interferes with my monorepo build. Even though the Storybook build works correctly, the presence of the warning causes the monorepo build to not be successful:

==[ SUCCESS WITH WARNINGS: 1 operation ]=======================================

--[ WARNING: storybook-app ]-----------------------[ 4 minutes 14.3 seconds ]--

The CJS build of Vite's Node API is deprecated. See https://vitejs.dev/guide/troubleshooting.html#vite-cjs-node-api-deprecated for more details.

Operations succeeded with warnings.

JReinhold commented 1 month ago

@kaiyoma did you try the workaround described in https://github.com/storybookjs/storybook/issues/26291#issuecomment-1978193283 ?

If that doesn't work, it would be great to get a minimal reproduction. We are aware of the issue with react-docgen-typescript, but if you're not using that then it would be helpful to understand what is triggering it for you.

kaiyoma commented 1 month ago

@kaiyoma did you try the workaround described in #26291 (comment) ?

@JReinhold I tried it, but this doesn't help either. Same error. Here's the trace, if this is helpful:

$ VITE_CJS_TRACE=true ds

> storybook-app@0.1.0 dev
> cross-env VITE_GEIGER_ENV=storybook storybook dev -c storybook-config --ci --port 9001

storybook v8.2.5

Trace: The CJS build of Vite's Node API is deprecated. See https://vitejs.dev/guide/troubleshooting.html#vite-cjs-node-api-deprecated for more details.
    at warnCjsUsage (C:\Users\kgetz\Work\event-viewer\common\temp\node_modules\.pnpm\vite@5.3.4_@types+node@20.11.30_less@4.2.0_lightningcss@1.25.1\node_modules\vite\index.cjs:33:3)
    at Object.<anonymous> (C:\Users\kgetz\Work\event-viewer\common\temp\node_modules\.pnpm\vite@5.3.4_@types+node@20.11.30_less@4.2.0_lightningcss@1.25.1\node_modules\vite\index.cjs:3:1)
    at Module._compile (node:internal/modules/cjs/loader:1376:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1435:10)
    at Object.newLoader (C:\Users\kgetz\Work\event-viewer\common\temp\node_modules\.pnpm\esbuild-register@3.5.0_esbuild@0.21.5\node_modules\esbuild-register\dist\node.js:2262:9)
    at extensions..js (C:\Users\kgetz\Work\event-viewer\common\temp\node_modules\.pnpm\esbuild-register@3.5.0_esbuild@0.21.5\node_modules\esbuild-register\dist\node.js:4838:24)
    at Module.load (node:internal/modules/cjs/loader:1207:32)
    at Module._load (node:internal/modules/cjs/loader:1023:12)
    at Module.require (node:internal/modules/cjs/loader:1235:19)
    at require (node:internal/modules/helpers:176:18)
info => Starting manager..
info => Starting preview..

I actually just realized that Vite offers an escape hatch for suppressing the warning (VITE_CJS_IGNORE_WARNING=true) so that will unblock me for now. It sounds like this issue will eventually get fixed in Storybook, based on the comments above. Just as long as it gets fixed before Vite removes the CJS API entirely. 😄

storybookjs / storybook