Vite: Support Vite 5

[IanVS]

Closes #24333

What I did

This gets us ready for Vite 5.

• Adds ^5.0.0 to the peer dependencies
• Installs the Vite 5 beta in the sandboxes
• Updates `@joshwooding/vite-plugin-react-docgen-typescript to a version that also allows vite 5 in its peer deps
• Use dynamic imports of all Vite functions, to force the use of ESM, since they are deprecating their CJS methods.
• Update the monorepo to use Vite 5 as a dev dependency. Not really possible because Jest cannot handle ESM easily. Need to stay on Vite 4 until we can migrate to vitest.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

• stories
• unit tests
• integration tests
• end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

I think the sandboxes should cover this, but it would be great if we could cut a canary for this change, and then test it out manually in projects using vite version 3 through 5, just to be sure the dynamic imports did not somehow break something.

Documentation

• Add or update documentation reflecting your changes
• If you are deprecating/removing a feature, make sure to update
  MIGRATION.MD

Checklist for Maintainers

• When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

• Make sure this PR contains one of the labels below:

  Available labels

  • bug: Internal changes that fixes incorrect behavior.
  • maintenance: User-facing maintenance tasks.
  • dependencies: Upgrading (sometimes downgrading) dependencies.
  • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
  • cleanup: Minor cleanup style change. Will not show up in release changelog.
  • documentation: Documentation only changes. Will not show up in release changelog.
  • feature request: Introducing a new feature.
  • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
  • other: Changes that don't fit in the above categories.

🦋 Canary release

This pull request has been released as version 0.0.0-pr-24395-sha-2aacb587. Install it by pinning all your Storybook dependencies to that version.

More information

Published version	0.0.0-pr-24395-sha-2aacb587
Triggered by	@ndelangen
Repository	storybookjs/storybook
Branch	vite-5
Commit	2aacb587
Datetime	Mon Oct 9 14:29:22 UTC 2023 (1696861762)
Workflow run	6458019422

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=24395

[socket-security]

Updated dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
@joshwooding/vite-plugin-react-docgen-typescript	0.2.1...0.3.0	None	+0/-0	24 kB	joshwooding

[IanVS]

Hmmm, I wonder what this error means:

- Installing Storybook dependencies

An error occurred while installing dependencies:
YARN2 error

Please check the logfile generated at ./storybook.log for troubleshooting and try again.

     Error: YARN2 error

Please check the logfile generated at ./storybook.log for troubleshooting and try again.
    at Yarn2Proxy.addDependencies (/tmp/storybook/code/lib/cli/dist/generate.js:55:75)
    at async baseGenerator (/tmp/storybook/code/lib/cli/dist/generate.js:77:1867)
    at async generator16 (/tmp/storybook/code/lib/cli/dist/generate.js:109:2509)
    at async installStorybook (/tmp/storybook/code/lib/cli/dist/generate.js:109:6989)
    at async doInitiate (/tmp/storybook/code/lib/cli/dist/generate.js:128:845)
    at async withTelemetry (/tmp/storybook/code/lib/core-server/dist/index.js:103:3903)
    at async initiate (/tmp/storybook/code/lib/cli/dist/generate.js:140:250)
An error occurred while executing: `node /tmp/storybook/code/lib/cli/bin/index.js init --yes --debug`
🚨 Initializing Storybook failed

It looks like that logfile isn't archived as an artifact (maybe it should be?). And it's not failing locally for me...

[yannbf]

Thanks a lot @IanVS! Re: vitest we are actually doing a migration to it, with currently 2k tests passing and only 50 failing. Almost there! cc @ndelangen

Regarding the error, we definitely should store the artifact. However I think you figured it out and fixed it already 👍

[JReinhold]

I wonder if the changed Chromatic stories are actually valid here, and not just flakes. There's a bunch of fonts changed, which we've experienced before to a lesser degree, but then there's also this which looks very different: https://www.chromatic.com/test?appId=63b588a512565bfaace15e7c&id=65202d1f6d601fdca7587360

[ndelangen]

@IanVS canary release ongoing: https://github.com/storybookjs/storybook/actions/runs/6458019422

[IanVS]

@joshwooding, @valentinpalkovic I've returned to just using version 0.3.0, which added peerDependencies support for Vite 5. I think updating to 0.3.1 should be a PR on its own, so we can isolate any possible performance impacts.

[IanVS]

@valentinpalkovic you asked why tests aren't failing since vite 5 requires node 18. Am I remembering correctly that changes to sandbox-templates.ts don't take effect until after the PR is merged and the new sandboxes are created in https://github.com/storybookjs/sandboxes?

If that's the case, then I'm not sure how we can test Vite 5 at all in our CI.

[JReinhold]

If that's the case, then I'm not sure how we can test Vite 5 at all in our CI.

A fix for now could be to create a new sandbox for Vite 5 that has inDevelopment: true so it will be skipped by everything. Then once we've figured out what to do with Node 18 and CI in general, we can revisit all the Vite sandboxes like this PR does.

However if we figure out CI and Node 18 soon, the temporary solution might not be worth it.

@valentinpalkovic

[IanVS]

@valentinpalkovic @JReinhold I believe the CI suite is now running node 18, correct? Does anything else need to be done before this can be merged?

[valentinpalkovic]

Go ahead. I don't see anything stopping us from merging this. Maybe getting a final approval from @ndelangen since he looked already at the PR.

[IanVS]

Hm, I updated it on the base branch, and now CI is failing. Bench fails with:

Cannot read properties of null (reading 'page')

[JReinhold]

Awesome work! 💪

[JReinhold]

I tested it out in our UI Storybook, where we get the CJS warning because we import mergeConfig top-level in main.ts. Moving that import to a dynamic import fixes that warning, but it tells us that any user that imports from Vite in main.ts will still use the CJS bundle, which is a shame. This PR is still an improvement though.

This will be more critical when CJS is actually gone in Vite 6 though, as that will break anyone's usage of Vite in their main.ts files. This is just another argument for moving Storybook to ESM-only sooner rather than later.

[IanVS]

it tells us that any user that imports from Vite in main.ts will still use the CJS bundle

Yeah, unless they have "type": "module" in their package.json or are using main.mts (and a modern TS setup). But, that's at least something they have control over.

Thanks for the review/merge!

[rasgo-cc]

Should the warning not show up already? I'm on storybook 7.5.3 but still get it 🤔

➜ yarn storybook dev
@storybook/cli v7.5.3

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.

Any idea what's happening @IanVS @yannbf ?

[IanVS]

No, I'm not sure why it's happening, and unfortunately the stack trace when using VITE_CJS_TRACE=true isn't very helpful either. I could have sworn this was working when I merged this PR, but since I wasn't able to add tests due to limitations in our testing suite, it's hard to say when/why this broke.

The good news is that the message can be ignored for now. The removal of the CJS API will not happen for a while longer, and there has been talk of moving Storybook itself to ESM, which would solve the issue. But if anyone has a chance to dig in and find out what's happening, so we can avoid the warning in the meantime, that would be awesome.