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

CLI: Re-Add Nuxt support #28607

Open
wants to merge 16 commits into
base: next
Choose a base branch
from
Open

Conversation

valentinpalkovic
Copy link
Contributor

@valentinpalkovic valentinpalkovic commented Jul 15, 2024

What I did

Reverts #28479

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!

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-28607-sha-b9bf7d39. Try it out in a new sandbox by running npx storybook@0.0.0-pr-28607-sha-b9bf7d39 sandbox or in an existing project with npx storybook@0.0.0-pr-28607-sha-b9bf7d39 upgrade.

More information
Published version 0.0.0-pr-28607-sha-b9bf7d39
Triggered by @valentinpalkovic
Repository storybookjs/storybook
Branch revert-28479-revert/26884
Commit b9bf7d39
Datetime Mon Jul 15 13:08:23 UTC 2024 (1721048903)
Workflow run 9939992740

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=28607

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 78 MB 78 MB 0 B 1.11 0%
initSize 143 MB 143 MB 654 B 1.14 0%
diffSize 65.1 MB 65.1 MB 654 B 1.86 0%
buildSize 6.87 MB 6.87 MB 0 B 0.7 0%
buildSbAddonsSize 1.51 MB 1.51 MB 0 B 0.5 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 1.9 MB 1.9 MB 0 B 0.66 0%
buildSbPreviewSize 271 kB 271 kB 0 B - 0%
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 3.88 MB 3.88 MB 0 B 0.65 0%
buildPreviewSize 3 MB 3 MB 0 B 1.22 0%
testBuildSize 0 B 0 B 0 B - -
testBuildSbAddonsSize 0 B 0 B 0 B - -
testBuildSbCommonSize 0 B 0 B 0 B - -
testBuildSbManagerSize 0 B 0 B 0 B - -
testBuildSbPreviewSize 0 B 0 B 0 B - -
testBuildStaticSize 0 B 0 B 0 B - -
testBuildPrebuildSize 0 B 0 B 0 B - -
testBuildPreviewSize 0 B 0 B 0 B - -
name before after diff z %
createTime 6.5s 6.8s 247ms -1.06 3.6%
generateTime 19.9s 19.4s -519ms -1.18 -2.7%
initTime 13.9s 14.4s 496ms -1.06 3.4%
buildTime 7.8s 8.4s 557ms -0.5 6.6%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 5s 5.3s 318ms -1.03 5.9%
devManagerResponsive 3.2s 3.2s -15ms -1.34 -0.5%
devManagerHeaderVisible 515ms 491ms -24ms -1.34 -4.9%
devManagerIndexVisible 549ms 548ms -1ms -1.36 -0.2%
devStoryVisibleUncached 614ms 849ms 235ms -0.75 27.7%
devStoryVisible 547ms 547ms 0ms -1.36 0%
devAutodocsVisible 449ms 439ms -10ms -1.37 -2.3%
devMDXVisible 455ms 495ms 40ms -0.57 8.1%
buildManagerHeaderVisible 519ms 501ms -18ms -1.06 -3.6%
buildManagerIndexVisible 540ms 524ms -16ms -0.96 -3.1%
buildStoryVisible 521ms 500ms -21ms -1.05 -4.2%
buildAutodocsVisible 421ms 438ms 17ms -0.91 3.9%
buildMDXVisible 413ms 394ms -19ms -1.29 -4.8%

Greptile Summary

This PR reverts the removal of Nuxt support in Storybook, restoring functionality that was previously removed due to QA issues before the 8.2 release.

  • Added Nuxt framework detection and configuration in detect.ts and project_types.ts, using @storybook-vue/nuxt package
  • Added Nuxt generator with specific configuration in generators/NUXT/index.ts, including @nuxtjs/storybook module integration
  • Added Nuxt sandbox template nuxt-vite/default-ts marked as inDevelopment with Vite builder
  • Added special case handling for Nuxt in Yarn configuration to avoid log filtering
  • Added TypeScript detection for Nuxt projects via tsconfig.json even without direct TypeScript dependency

The changes appear comprehensive but lack test coverage and documentation updates that should be addressed before merging.

@valentinpalkovic valentinpalkovic changed the title Revert "Revert: Nuxt support" CLI: Add Nuxt support Jul 15, 2024
@valentinpalkovic valentinpalkovic changed the title CLI: Add Nuxt support CLI: Re-Add Nuxt support Jul 15, 2024
@valentinpalkovic valentinpalkovic self-assigned this Jul 15, 2024
Copy link

nx-cloud bot commented Jul 15, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit bf7484a. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@valentinpalkovic
Copy link
Contributor Author

@tobiasdiez
I am encountering the following error after init while testing the revert:

  1. npx nuxi@latest init (npm)
  2. cd ./nuxt-app
  3. npx storybook@0.0.0-pr-28607-sha-b9bf7d39 init
Running Storybook

> storybook
> storybook dev -p 6006 --quiet

storybook v0.0.0-pr-28607-sha-b9bf7d39

WARN The following packages are incompatible with Storybook 0.0.0-pr-28607-sha-b9bf7d39 as they depend on different major versions of Storybook packages:
WARN - @nuxtjs/storybook@8.1.5
WARN  Repo: https://storybook.nuxtjs.org
WARN 
WARN 
WARN Please consider updating your packages or contacting the maintainers for compatibility details.
WARN For more on Storybook 8 compatibility, see the linked GitHub issue:
WARN https://github.com/storybookjs/storybook/issues/26031
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.
Error: Cannot find module '@storybook/builder-vite'
Require stack:
- ./node_modules/@storybook/core/dist/core-server/index.cjs
- ./node_modules/storybook/dist/generate.cjs
- ./node_modules/storybook/bin/index.cjs
    at Module._resolveFilename (node:internal/modules/cjs/loader:1140:15)
    at Module._resolveFilename (./node_modules/esbuild-register/dist/node.js:4799:36)
    at Function.resolve (node:internal/modules/helpers:188:19)
    at getPreviewBuilder (./node_modules/@storybook/core/dist/core-server/index.cjs:102320:19)
    at buildDevStandalone (./node_modules/@storybook/core/dist/core-server/index.cjs:104759:5)
    at async withTelemetry (./node_modules/@storybook/core/dist/core-server/index.cjs:103500:12)
    at async dev (./node_modules/storybook/dist/generate.cjs:735:506)
    at async Command.<anonymous> (./node_modules/storybook/dist/generate.cjs:737:245)

WARN Broken build, fix the error above.
WARN You may need to refresh the browser.

@valentinpalkovic
Copy link
Contributor Author

@tobiasdiez
I am encountering the browser following error after init while testing the revert:

npx nuxi@latest init (pnpm)
cd ./nuxt-app
npx storybook@0.0.0-pr-28607-sha-b9bf7d39 init
Running Storybook

SyntaxError: The requested module '/node_modules/.pnpm/lodash@4.17.21/node_modules/lodash/kebabCase.js?v=726791be' does not provide an export named 'default'

@ndelangen This might be CPC related, though.

ndelangen added a commit that referenced this pull request Jul 15, 2024
@ndelangen ndelangen mentioned this pull request Jul 15, 2024
8 tasks
@tobiasdiez
Copy link
Contributor

Thanks for this PR and your tests!

Error: Cannot find module '@storybook/builder-vite'
Require stack:

  • ./node_modules/@storybook/core/dist/core-server/index.cjs
  • ./node_modules/storybook/dist/generate.cjs
  • ./node_modules/storybook/bin/index.cjs
    at Module._resolveFilename (node:internal/modules/cjs/loader:1140:15)
    at Module._resolveFilename (./node_modules/esbuild-register/dist/node.js:4799:36)
    at Function.resolve (node:internal/modules/helpers:188:19)
    at getPreviewBuilder (./node_modules/@storybook/core/dist/core-server/index.cjs:102320:19)
    at buildDevStandalone (./node_modules/@storybook/core/dist/core-server/index.cjs:104759:5)
    at async withTelemetry (./node_modules/@storybook/core/dist/core-server/index.cjs:103500:12)
    at async dev (./node_modules/storybook/dist/generate.cjs:735:506)
    at async Command. (./node_modules/storybook/dist/generate.cjs:737:245)

I can reproduce it as well. The problem is that in our setup @storybook/builder-vite is only a dependency of the nuxt module, but require is called from the main storybook (or core), and thus only looks for it in the main node_modules folder.

Simplest solution: just add @storybook/builder-vite as a main dependency. But then the user has to sync the builder-vite version with what the nuxt module is using (or we declare it as peer-dep). Would work but maybe not ideal

More involved solution: make it possible that presets return a javascript object as builder, instead of only a module name. (Similar to how eslint v9 is handling plugins now).

Do you have any preference, or another idea?

@tobiasdiez
Copy link
Contributor

@tobiasdiez I am encountering the browser following error after init while testing the revert:

npx nuxi@latest init (pnpm) cd ./nuxt-app npx storybook@0.0.0-pr-28607-sha-b9bf7d39 init Running Storybook

SyntaxError: The requested module '/node_modules/.pnpm/lodash@4.17.21/node_modules/lodash/kebabCase.js?v=726791be' does not provide an export named 'default'

@ndelangen This might be CPC related, though.

I've fixed this already in the main branch by adding lodash to optimizeDeps:
https://github.com/nuxt-modules/storybook/blob/64e80dec7c15d9e03fa22f806ac269fbe8146d1f/packages/storybook-addon/src/preset.ts#L171-L175

@valentinpalkovic
Copy link
Contributor Author

Thanks for this PR and your tests!

Error: Cannot find module '@storybook/builder-vite'
Require stack:

  • ./node_modules/@storybook/core/dist/core-server/index.cjs
  • ./node_modules/storybook/dist/generate.cjs
  • ./node_modules/storybook/bin/index.cjs
    at Module._resolveFilename (node:internal/modules/cjs/loader:1140:15)
    at Module._resolveFilename (./node_modules/esbuild-register/dist/node.js:4799:36)
    at Function.resolve (node:internal/modules/helpers:188:19)
    at getPreviewBuilder (./node_modules/@storybook/core/dist/core-server/index.cjs:102320:19)
    at buildDevStandalone (./node_modules/@storybook/core/dist/core-server/index.cjs:104759:5)
    at async withTelemetry (./node_modules/@storybook/core/dist/core-server/index.cjs:103500:12)
    at async dev (./node_modules/storybook/dist/generate.cjs:735:506)
    at async Command. (./node_modules/storybook/dist/generate.cjs:737:245)

I can reproduce it as well. The problem is that in our setup @storybook/builder-vite is only a dependency of the nuxt module, but require is called from the main storybook (or core), and thus only looks for it in the main node_modules folder.

Simplest solution: just add @storybook/builder-vite as a main dependency. But then the user has to sync the builder-vite version with what the nuxt module is using (or we declare it as peer-dep). Would work but maybe not ideal

More involved solution: make it possible that presets return a javascript object as builder, instead of only a module name. (Similar to how eslint v9 is handling plugins now).

Do you have any preference, or another idea?

@tobiasdiez Take a look at https://github.com/storybookjs/storybook/blob/next/code/frameworks/angular/src/preset.ts#L31. You can see that you can also just pass an absolute path as a value. With this approach, you make sure that @storybook/builder-vite or any other package which you have installed as a dependency will be resolved from node_modules/@storybook-vue/nuxt instead of node_modules/@storybook/core-preset

@tobiasdiez
Copy link
Contributor

Many thanks. This worked like a charm! 🚀

In nuxt-modules/storybook#729, I've now also added e2e tests (using this PR here to add storybook to a nuxt starter). Running build-storybook now works (and is tested), but I still get some errors when actually running the storybook. Is there a reusable test suite or something like this that I can point to a storybook-static folder and that verifies that the stories are okay. In a similar vain, it would be handy to have the storybook templates used for the tests here somehow published so that they can be reused to test downstream packages (I noticed this need not only for the nuxt integration but also while developing the vue addon https://github.com/tobiasdiez/storybook-vue-addon).

@kasperpeulen
Copy link
Contributor

@valentinpalkovic @tobiasdiez Maybe it is best if we have Nuxt sandbox over here:

@valentinpalkovic
Copy link
Contributor Author

@tobiasdiez As Kasper mentioned, we could add Nuxt to our sandboxes, and you could test the sandbox on this PR and rerun it every time you release a new Canary version. Otherwise, I don't see an easy way to copy over our test suite setup to your repository.

@KeremTurkyilmaz
Copy link

Hello @tobiasdiez do you have any news when this draft will be merged ?

@tobiasdiez
Copy link
Contributor

@kasperpeulen @valentinpalkovic Thanks for the suggestion. Adding a sandbox would indeed give some confidence that things are working, but I was hoping for a solution that we can apply in downstream projects (e.g. that PRs to the nuxt storybook addon are not breaking things). Similar to https://www.npmjs.com/package/@apollo/server-integration-testsuite.

@KeremTurkyilmaz We first need to fix the major bugs in the nuxt module. Help moving this forward is of course very welcome!

@valentinpalkovic
Copy link
Contributor Author

@tobiasdiez Unfortunately, we don't have a dedicated testsuite solution like Apollo does. :/

@ndelangen ndelangen self-assigned this Aug 14, 2024
@tobiasdiez
Copy link
Contributor

I've added a nuxt sandbox, but not sure if I did it correctly (it appears it is not run as part of the ci)

@valentinpalkovic
Copy link
Contributor Author

Hi @tobiasdiez

I get the following error when running yarn task --task sandbox --template nuxt-vite/default-ts --debug:

💃 adding stories
Error running task sandbox:
{
  "code": "ERR_INVALID_ARG_TYPE"
}

TypeError: The "paths[1]" argument must be of type string. Received null
    at new NodeError (node:internal/errors:405:5)
    at validateString (node:internal/validators:162:11)
    at resolve (node:path:1097:7)
    at addStories (/Users/valentinpalkovic/Projects/storybook-next/scripts/tasks/sandbox-parts.ts:589:25)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async Object.run (/Users/valentinpalkovic/Projects/storybook-next/scripts/tasks/sandbox.ts:82:7)
    at async runTask (/Users/valentinpalkovic/Projects/storybook-next/scripts/task.ts:316:24)
    at async run (/Users/valentinpalkovic/Projects/storybook-next/scripts/task.ts:473:28) {
  code: 'ERR_INVALID_ARG_TYPE'
}

I guess it has something to do with the Nuxt framework not providing default stories?

@tobiasdiez
Copy link
Contributor

We do have default stories in https://github.com/nuxt-modules/storybook/tree/main/packages/storybook-addon/template/cli, but I have no idea how they get installed nor why this would fail in the sandbox. If you give me some pointers where to start, I can look into this.

@tobiasdiez
Copy link
Contributor

tobiasdiez commented Oct 6, 2024

Can we please get this in as soon as possible? We constantly have people opening issues at the nuxt-storybook module because they configured something wrong. Having the cli integration would make setting this up with nuxt way easier and robust. 🙇

The problem with the sandbox might come from the fact that we don't add the stories under the stories folder but under components

@tobiasdiez
Copy link
Contributor

@storybookjs/core, we would greatly appreciate support from the core team regarding the Nuxt-Storybook integration. @chakAs3 and I are working on this in our free time, and unfortunately, we don't have the resources to continually address new issues that arise in the Nuxt module due to upstream changes in Storybook. Many of these issues could be avoided if the Nuxt integration sandbox were included in your test suite.

Additionally, our installation instructions are more complex than necessary because the Storybook CLI still doesn't fully recognize Nuxt projects. Thus new users make mistakes during the setup, and then open issues at the nuxt repo adding to our workload. It would be incredibly helpful if we could get this PR merged as soon as possible.

Could you also review my pending PRs at your earliest convenience? (https://github.com/storybookjs/storybook/pulls/tobiasdiez) If you believe my approach in a PR isn't the right solution (e.g., PR #29172 comment or PR #28595), that’s completely understandable. However, it would be immensely helpful if you would then implement an alternative solution (or at least suggest one). These PRs are not just for theoretical improvements; they address critical issues that make maintaining the Nuxt integration extremely challenging.

Thank you for your time and consideration!

@valentinpalkovic
Copy link
Contributor Author

I have informed the core team and we will try to prioritise this and the mentioned PRs asap.

@chakAs3
Copy link
Contributor

chakAs3 commented Oct 16, 2024

Yes, this has become quite challenging for users. Just last week, I helped a company manually set up Storybook on a large Nuxt project. I had previously built a custom CLI for this, but stopped maintaining it due to the difficulty of keeping it in sync with Storybook's frequent releases.

@tobiasdiez, on a related note, I’m encountering consistent failures when building and publishing Storybook to Chromatic, likely due to the large number of components and insufficient RAM. Have you come across this issue, and is there a recommended solution?

@tobiasdiez
Copy link
Contributor

@tobiasdiez, on a related note, I’m encountering consistent failures when building and publishing Storybook to Chromatic, likely due to the large number of components and insufficient RAM. Have you come across this issue, and is there a recommended solution?

I've never experienced such issues with the chromatic-workflow setup in the storybook-nuxt-module repo. I would love to migrate my own projects to the nuxt-module (and thus gain more experience with publishing it to chromatic), but that's currently blocked by various bugs in Storybook.

@valentinpalkovic
Copy link
Contributor Author

I talked to the team, and currently, we are wrapping up the 8.4 release. Next week, I will get the chance to take a proper look and hopefully get the PR merged.

@chakAs3
Copy link
Contributor

chakAs3 commented Oct 16, 2024

chromatic-workflow setup in the storybook-nuxt-module

It works fine on small or medium-sized projects i guess, but the one I’m working on now has hundreds of components, i only started by creating stories for few components. I’m wondering if Storybook’s build process is handling all the components, even those without stories, when generating the static files ? which causing the memory issue

@valentinpalkovic
Copy link
Contributor Author

Finally taking a look...

@valentinpalkovic valentinpalkovic marked this pull request as ready for review October 28, 2024 11:51
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

17 file(s) reviewed, 19 comment(s)
Edit PR Review Bot Settings | Greptile

code/core/src/cli/detect.test.ts Show resolved Hide resolved
code/core/src/cli/detect.ts Show resolved Hide resolved
code/core/src/cli/detect.ts Show resolved Hide resolved
code/core/src/cli/project_types.ts Show resolved Hide resolved
code/core/src/types/modules/renderers.ts Show resolved Hide resolved
code/lib/cli-storybook/src/sandbox-templates.ts Outdated Show resolved Hide resolved
code/lib/cli-storybook/src/sandbox-templates.ts Outdated Show resolved Hide resolved
code/lib/create-storybook/src/generators/NUXT/index.ts Outdated Show resolved Hide resolved
@valentinpalkovic
Copy link
Contributor Author

Hi @tobiasdiez

Our CI is finally green. There is one caveat, though. When generating a sandbox, it does not contain our default stories (Button, Header, Page), which some of our e2e-production tests require. Therefore, I was forced to disable our e2e-production tests on the nuxt sandbox. If you add the default stories to https://github.com/nuxt-modules/storybook/tree/main/packages/storybook-addon/template/cli, we may be able to reactivate the e2e-production tests.

@kasperpeulen kasperpeulen self-assigned this Nov 5, 2024
@tobiasdiez
Copy link
Contributor

Our CI is finally green.

Awesome! Thanks for you work!

When generating a sandbox, it does not contain our default stories (Button, Header, Page), which some of our e2e-production tests require. Therefore, I was forced to disable our e2e-production tests on the nuxt sandbox. If you add the default stories to https://github.com/nuxt-modules/storybook/tree/main/packages/storybook-addon/template/cli, we may be able to reactivate the e2e-production tests.

I'm not so sure if that's a good idea. Sure, for e2e tests it makes a lot of sense to have these standard stories. However, as a user of the nuxt module these auto-generated files are mostly noise and I think most users would immediately delete them.

Is it possible that you use the vue-package to generate the stories for the e2e tests here in this repo?

(By the way, it would perhaps be an improvement for the on-boarding if instead of auto-generating Buttons and similar controls that are not really useful, these would be provided by the storybook package - similarly, how nuxt provides the welcome screen that we show in the storybook. Then storybook init would only generate one or two stories that show users how everything works.)

@kasperpeulen
Copy link
Contributor

However, as a user of the nuxt module these auto-generated files are mostly noise and I think most users would immediately delete them.

They are meant to be serve as examples stories that once you have looked, you can delete indeed.
cc @shilman About our product vision for shipping with examples stories.

(By the way, it would perhaps be an improvement for the on-boarding if instead of auto-generating Buttons and similar controls that are not really useful, these would be provided by the storybook package - similarly, how nuxt provides the welcome screen that we show in the storybook. Then storybook init would only generate one or two stories that show users how everything works.)

Can you elaborate on that? I don't really follow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Empathy Backlog
Development

Successfully merging this pull request may close these issues.

6 participants