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

chore: merge storybook-nuxt repo #594

Merged
merged 165 commits into from
May 29, 2024
Merged

Conversation

tobiasdiez
Copy link
Collaborator

@tobiasdiez tobiasdiez commented May 12, 2024

Merge the storybook addon () into this repo while preserving the commit history.

This is needed as #592 will integrate the nuxt module closer with the storybook addon, and thus more often simultaneous changes are necessary. Moreover, we don't need to replicate ci and testing infrastructure.

This PR only adds the code and making ci pass. Adding the proper workspace integration and ci builds will be done as follow-ups.

Please merge this PR and not squash it since otherwise the old commit history is lost.

Copy link

netlify bot commented May 12, 2024

👷 Deploy request for nuxt-storybook pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 35e79ed

@tobiasdiez tobiasdiez requested a review from chakAs3 May 12, 2024 09:36
@tobiasdiez
Copy link
Collaborator Author

@chakAs3 what do you think?

@chakAs3
Copy link
Collaborator

chakAs3 commented May 14, 2024

@chakAs3 what do you think?

Honestly, making maintenance simpler may seem like a good idea, but it's not quite that simple. The Storybook preset @storybook-vue/nuxt doesn't depend on the Nuxt module, and it shouldn't.

The preset ( addon ) does its own thing in Storybook, while the Nuxt module extends Nuxt app functionalities.

Updating the Nuxt module doesn't affect the Storybook preset.

Usually, we'd follow the same process as with the Next.js preset. Once it's stable, we'll move it to the Storybook monorepo.

I still recommend keeping @storybook/nuxt in a separate repository, linked to nuxt-modules/storybook as a package. This aligns with the practice followed by the Nuxt team for their Nuxi CLI, which is no longer part of the monorepo.

@tobiasdiez
Copy link
Collaborator Author

Honestly, making maintenance simpler may seem like a good idea, but it's not quite that simple. The Storybook preset @storybook-vue/nuxt doesn't depend on the Nuxt module, and it shouldn't.

It was not definitely not my intention to change this. The Storybook addon should still be independent of the Nuxt module. I only would like to have them in the same repository so that

  • There is one place to open issues (there were quite a few duplicates and for an average use it's not clear if it's a bug with the nuxt module or with the storybook addon)
  • We don't have to duplicate ci infrastructure, eslint + prettier config, etc
  • It's easier to make changes that effect both the addon and the nuxt module
  • Currently it is possible to break something in the Storybook addon and only realize it once one tries to upgrade the package here in the Nuxt module repo (e.g. because only the tailwind integration is no longer working).

Once the Storybook addon is more stable, we can indeed move it to the storybook repo if they are interested. There is no problem with that.

Copy link
Collaborator

chakAs3 commented May 29, 2024

@tobiasdiez, we're good on this. I'm completely aligned with you. However, I can't create a merge commit for this; the only option is to squash. That's fine with me since it's still a preview version, and we need to stabilize it more before it's production-ready.

@tobiasdiez tobiasdiez merged commit a5827eb into nuxt-modules:main May 29, 2024
10 checks passed
@tobiasdiez tobiasdiez deleted the merge branch May 29, 2024 09:10
@tobiasdiez
Copy link
Collaborator Author

Awesome! Thanks for the feedback. I'll work on the ci integration etc in the coming days.

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 this pull request may close these issues.

4 participants