-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Maintenance: Merge @storybook/core
with storybook
#30168
base: next
Are you sure you want to change the base?
Conversation
View your CI Pipeline Execution ↗ for commit 2e9cfd8.
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
622 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings | Greptile
); | ||
} | ||
|
||
throw error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Re-throwing error prevents proper error reporting to users. Consider handling the error gracefully instead of throwing
code/core/src/bin/index.ts
Outdated
child.on('exit', (code) => { | ||
if (code != null) { | ||
process.exit(code); | ||
} | ||
process.exit(1); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: The exit handler will always call process.exit(1) after the conditional exit, which means non-zero exit codes will work but zero (success) codes will still exit with 1
}) | ||
); | ||
}); | ||
}); | ||
|
||
describe('removeDependencies', () => { | ||
it('with devDep it should run `npm uninstall @storybook/core`', async () => { | ||
it('with devDep it should run `npm uninstall storybook`', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: test description mentions 'npm uninstall' but is testing PNPM's remove command
@@ -25,7 +25,7 @@ vi.mock('@storybook/global', () => ({ | |||
}, | |||
})); | |||
|
|||
vi.mock('@storybook/core/client-logger'); | |||
vi.mock('storybook/internal/client-logger'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Duplicate mock declaration - this same mock appears again on line 49
'storybook/internal/core-errors': '__STORYBOOK_CORE_EVENTS__', | ||
'@storybook/core-events': '__STORYBOOK_CORE_EVENTS__', | ||
'@storybook/core/core-events': '__STORYBOOK_CORE_EVENTS__', | ||
'storybook/internal/core-events': '__STORYBOOK_CORE_EVENTS__', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: duplicate mapping for core-events - both 'storybook/internal/core-errors' and 'storybook/internal/core-events' map to 'STORYBOOK_CORE_EVENTS'
'storybook/internal/core-errors': EVENTS, | ||
'@storybook/core-events': EVENTS, | ||
'@storybook/core/core-events': EVENTS, | ||
'storybook/internal/core-events': EVENTS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: inconsistent naming - 'core-errors' vs 'core-events' for same EVENTS module
vi.mock('storybook/internal/channels', async (importOriginal) => { | ||
return { | ||
...(await importOriginal<typeof import('@storybook/core/channels')>()), | ||
...(await importOriginal<typeof import('storybook/internal/channels')>()), | ||
createBrowserChannel: () => mockChannel, | ||
}; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: duplicate mock definition - this mock is defined again on lines 46-51
@@ -1,6 +1,6 @@ | |||
// FIXME: breaks builder-vite, remove this in 7.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: this FIXME comment references version 7.0 but should be updated since this is targeting version 9.0
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
Before | After | Difference | |
---|---|---|---|
Dependency count | 54 | 53 | 🎉 -1 🎉 |
Self size | 22 KB | 19.07 MB | 🚨 +19.05 MB 🚨 |
Dependency size | 33.51 MB | 14.43 MB | 🎉 -19.08 MB 🎉 |
Bundle Size Analyzer | Link | Link |
sb
Before | After | Difference | |
---|---|---|---|
Dependency count | 55 | 54 | 🎉 -1 🎉 |
Self size | 1 KB | 1 KB | 0 B |
Dependency size | 33.53 MB | 33.50 MB | 🎉 -30 KB 🎉 |
Bundle Size Analyzer | Link | Link |
@storybook/cli
Before | After | Difference | |
---|---|---|---|
Dependency count | 396 | 395 | 🎉 -1 🎉 |
Self size | 496 KB | 496 KB | 🚨 +64 B 🚨 |
Dependency size | 75.42 MB | 75.39 MB | 🎉 -30 KB 🎉 |
Bundle Size Analyzer | Link | Link |
create-storybook
Before | After | Difference | |
---|---|---|---|
Dependency count | 112 | 111 | 🎉 -1 🎉 |
Self size | 1.11 MB | 1.11 MB | 0 B |
Dependency size | 42.64 MB | 42.61 MB | 🎉 -30 KB 🎉 |
Bundle Size Analyzer | Link | Link |
@kasperpeulen I'd love your thoughts on this approach, and how viable you see this PR as "done" for 9.0 release. @JReinhold Perhaps you could also have a look into this and see if there's something additional you'd want to do... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What I did
This is a proposal of how we could merge the
lib/cli
andcore
package into 1.This PR is a WIP.
lib/cli
core
'spackage.json
'sname
-field tostorybook
@storybook/core
should now becomestorybook/internal
I am optimistic that this won't be too breaking for end-users, though it would make sense to release this as part of
9.0
, when we also remove the shim packages.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal
,ci:merged
orci:daily
GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/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 PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/core
team here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>
Greptile Summary
This PR proposes a significant architectural change to merge the
lib/cli
andcore
packages into a single package namedstorybook
, with core functionality accessed throughstorybook/internal
.@storybook/core
tostorybook
and moves all core functionality understorybook/internal/*
paths@storybook/core/*
tostorybook/internal/*
lib/cli
package fromstorybook
tostorybook-renamed
to avoid conflictscode/core/bin/index.cjs
with Node.js version check and error handling@storybook/core
dependencies and updates package resolutions to point to the new unified packageThe changes are extensive but focused on package organization and import paths, with minimal functional changes. This is planned for Storybook 9.0 release.