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

wip: Storybook 8 #2816

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

Conversation

talentlessguy
Copy link
Contributor

@talentlessguy talentlessguy commented Sep 26, 2024

based on #2674

What I did

  1. Upgraded to Storybook 8 everywhere

Copy link

changeset-bot bot commented Sep 26, 2024

🦋 Changeset detected

Latest commit: 723ad59

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@bashmish
Copy link
Member

bashmish commented Sep 26, 2024

there are pipeline errors, so those are the minimum that such migration need to cover

in general our Storybook pipeline is pretty reliable in testing even such big changes like this PR, but a few things I always needed to check manually, specifically everything related to path resolution, because paths work very differently when you test within a subpackage which is part of a bigger monorepo like modern-web and inside a simple project, also differently when it's an actual published NPM package and when it's a symlink, so I'm afraid I'll need to conduct more tests manually after the pipeline is green

my team has been planning to work on this in this autumn, can't give specific timelines, but sooner than later
so we gonna have a closer look into your PR

@talentlessguy
Copy link
Contributor Author

talentlessguy commented Sep 26, 2024

Thank you for the review, i didnt have historical context of the monorepo, so i will revert some of the changes for unmaintained tools

Also i forgot to tell but this PR is work in progress

@talentlessguy talentlessguy changed the title Storybook 8 wip: Storybook 8 Sep 26, 2024
@@ -46,7 +46,8 @@
"@rocket/cli": "^0.10.1",
"@rocket/core": "^0.1.2",
"@rocket/launch": "^0.6.0",
"@rocket/search": "^0.6.0"
"@rocket/search": "^0.6.0",
"@storybook/core": "^8.3.3"

Choose a reason for hiding this comment

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

This shouldn't be needed, you should be able to depend on storybook instead, I hope

Copy link
Contributor Author

Choose a reason for hiding this comment

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

storybook itself includes a lot of unrelated stuff. I only need @storybook/core/channels

Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be in one of the packages, rather that the monorepo root?

@@ -2,19 +2,11 @@

export async function generateSetupAddonsScript() {
return `
import { createChannel as createPostMessageChannel } from '@storybook/channel-postmessage';
import { createChannel as createWebSocketChannel } from '@storybook/channel-websocket';
Copy link
Member

@bashmish bashmish Oct 3, 2024

Choose a reason for hiding this comment

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

did you remove these 2 deps? I assumed they moved stuff to core or reexport and then import from it in their own builders, is that so? if that, I think we don't need to depend on these 2 packages directly anymore

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