Core: Fix cyclical dependency in core addons#31750
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a cyclical dependency issue in core addons by removing empty exports and outdated module mappings while updating import paths.
- Removed now-unneeded files and exports for outline, measure, and component-testing.
- Updated import paths in csf/core-annotations to use relative references.
- Cleaned up configuration entries in the scripts and package.json files to eliminate obsolete alias mappings.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| code/core/src/outline/index.ts | Removed empty export to eliminate an unnecessary module. |
| code/core/src/measure/index.ts | Removed empty export to eliminate an unnecessary module. |
| code/core/src/csf/core-annotations.ts | Updated import paths from absolute to relative to reduce coupling. |
| code/core/src/component-testing/index.ts | Removed an empty module to resolve cyclical dependency. |
| code/core/src/backgrounds/index.ts | Removed empty export to resolve unused alias. |
| code/core/scripts/entries.ts | Removed definitions for removed modules to avoid dangling references. |
| code/core/scripts/dts.ts | Cleaned up type definition mappings corresponding to removed modules. |
| code/core/package.json | Removed outdated alias mappings to reflect module removals. |
Comments suppressed due to low confidence (2)
code/core/package.json:86
- The removal of legacy alias mappings for modules such as backgrounds, measure, and outline may affect external consumers. It is recommended to update the migration documentation to clearly state these breaking changes.
"./internal/backgrounds": {
code/core/scripts/entries.ts:22
- Removing these module definitions is key to fixing the cyclical dependency, but please ensure that any dependent tests or consumers are updated or notified, as this may be a breaking change.
define('src/backgrounds/index.ts', ['browser', 'node'], true, ['react'], [], [], true),
|
@dannyhw can you check if this canary fixes the cyclical issue? |
|
View your CI Pipeline Execution ↗ for commit d051121.
☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 49 | 49 | 0 |
| Self size | 31.88 MB | 31.85 MB | 🎉 -38 KB 🎉 |
| Dependency size | 17.41 MB | 17.41 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
sb
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 1 KB | 1 KB | 0 B |
| Dependency size | 49.29 MB | 49.25 MB | 🎉 -38 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 216 | 216 | 0 |
| Self size | 582 KB | 582 KB | 0 B |
| Dependency size | 94.66 MB | 94.57 MB | 🎉 -95 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 185 | 185 | 0 |
| Self size | 31 KB | 31 KB | 0 B |
| Dependency size | 78.74 MB | 78.70 MB | 🎉 -38 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 1 | 1 | 0 |
| Self size | 12.53 MB | 12.47 MB | 🎉 -58 KB 🎉 |
| Dependency size | 98 KB | 98 KB | 0 B |
| Bundle Size Analyzer | Link | Link |
There was a problem hiding this comment.
LGTM
8 files reviewed, no comments
Edit PR Review Bot Settings | Greptile
…s/storybook into jeppe/fix-preview-api-bicycle
Core: Fix cyclical dependency in core addons (cherry picked from commit 16a316c)
|
Hey @JReinhold Sorry I couldn't test the other day had a lot going on. Unfortunately react native actions was actually using the actions/preview entrypoint to setup actions, maybe there is a way around this but I would need some help to figure that out. edit: still looking into this, the main thing currently is it just breaking the build but I need to see if I can just remove that part Regarding the react compiler, it is still erroring and I still need to comment out the core annotations to get things to work, one thing I noticed though is that if I just comment out the core annotations in the list even without removing the import it works.
|
Closes #31573
What I did
preview-apibundle instead of a separate thing. the separate entry point was causing a cyclical dependency,storybook/preview-api->storybook/highlight/preview->storybook/preview-api/previewentry points, as they weren't actually useful for anything. They are automatically applied in the Storybook runtime as well as in portable stories, implicitly viasetProjectAnnotations().As discussed with @kasperpeulen, @yannbf, @ndelangen.
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:mergedorci:dailyGH 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.tsMake 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-31750-sha-3896d4de. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-31750-sha-3896d4de sandboxor in an existing project withnpx storybook@0.0.0-pr-31750-sha-3896d4de upgrade.More information
0.0.0-pr-31750-sha-3896d4dejeppe/fix-preview-api-bicycle3896d4de1749649961)To request a new release of this pull request, mention the
@storybookjs/coreteam.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=31750Greptile Summary
Resolves critical cyclical dependency issue in Storybook's core addons by restructuring how preview APIs are bundled and imported.
code/core/src/csf/core-annotations.ts/previewentry points fromcode/core/package.jsonfor various addons (backgrounds, measure, outline, etc.)code/core/src/(backgrounds, outline, measure, component-testing)code/core/scripts/entries.tsanddts.tsto remove preview entries from external dependenciessetProjectAnnotations(), fixing Metro bundler crashes in React Native