Fully bundle ws, semver and recast #32854
Conversation
📝 WalkthroughWalkthroughThree packages (recast, semver, ws) moved from dependencies to devDependencies in code/core/package.json. These packages are no longer designated as runtime dependencies and are now classified as development-only. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/package.json(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/package.json
🧠 Learnings (1)
📓 Common learnings
Learnt from: ndelangen
PR: storybookjs/storybook#32484
File: code/core/package.json:326-326
Timestamp: 2025-09-17T07:31:04.432Z
Learning: In Storybook's core package, dependencies like `open` are bundled into the final distribution during the build process, so they should remain in devDependencies rather than being moved to dependencies. End users don't need these packages as separate runtime dependencies since they're included in the bundled code.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/core/package.json (1)
197-335: Manual verification required: Prettier formatting cannot be verified in sandbox environment.The sandbox environment lacks dependencies and cannot execute Prettier. While the JSON appears well-formatted with consistent indentation, verify locally before committing:
cd code/core && npx prettier --check package.json # If changes needed: npx prettier --write package.jsonConfirm the file formatting aligns with your project's
.prettierrcconfiguration.
| "recast": "^0.23.5", | ||
| "semver": "^7.6.2", | ||
| "ws": "^8.18.0" | ||
| "esbuild": "^0.18.0 || ^0.19.0 || ^0.20.0 || ^0.21.0 || ^0.22.0 || ^0.23.0 || ^0.24.0 || ^0.25.0" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify no runtime code imports the moved packages before merging.
Moving recast, semver, and ws from dependencies to devDependencies is the correct change per the PR rationale—these should be development-only since they're bundled tools rather than runtime dependencies. However, this assumes:
- No runtime imports: Source code in the exported entry points must not import these packages at runtime.
- Build system fix in place: The bundler must be configured to prevent re-bundling these packages.
- No consumer impact: End users shouldn't be affected since these packages weren't meant to be runtime dependencies.
Please verify the following before merging:
Additionally, confirm that a corresponding build system fix exists in this PR or a related issue (#32822) to prevent these packages from being bundled. The PR description indicates that the build system had a bug causing these to be bundled despite only their types being exposed.
Based on learnings from PR #32484, bundled packages belong in devDependencies.
Also applies to: 313-313, 316-316, 333-333
🏁 Script executed:
#!/bin/bash
# Description: Verify recast, semver, ws are not imported in runtime source code
# Search for imports of moved packages in src/ directory
echo "=== Checking for runtime imports of moved packages ==="
echo ""
echo "Searching for 'recast' imports:"
rg -n "from ['\"]recast['\"]|import.*recast|require\(['\"]recast" src/
echo ""
echo "Searching for 'semver' imports in non-build code:"
rg -n "from ['\"]semver['\"]|import.*semver|require\(['\"]semver" src/ | grep -v "\.test\|\.spec\|test/"
echo ""
echo "Searching for 'ws' imports in non-build code:"
rg -n "from ['\"]ws['\"]|import.*ws['\"]|require\(['\"]ws['\"]" src/ | grep -v "\.test\|\.spec\|test/"Length of output: 320
🏁 Script executed:
#!/bin/bash
# First, find the structure under code/core/
echo "=== Finding source code structure in code/core/ ==="
fd -t d . code/core/ -max-depth 2
echo ""
echo "=== List of files in code/core/ ==="
ls -la code/core/ | head -20
echo ""
echo "=== Checking for src directory ==="
ls -la code/core/src/ 2>/dev/null || echo "src/ not found, checking other locations..."
echo ""
echo "=== Checking lib, dist, or index files ==="
find code/core/ -maxdepth 1 -type f \( -name "*.js" -o -name "*.ts" -o -name "index.*" \) 2>/dev/null | head -10Length of output: 4029
🏁 Script executed:
#!/bin/bash
# Search for imports in code/core/ and related directories
echo "=== Searching for 'recast' imports in code/core ==="
rg -n "recast" code/core/ --type ts --type tsx --type js --type jsx 2>/dev/null | grep -i "import\|require\|from" | head -20
echo ""
echo "=== Searching for 'semver' imports in code/core ==="
rg -n "semver" code/core/ --type ts --type tsx --type js --type jsx 2>/dev/null | grep -i "import\|require\|from" | head -20
echo ""
echo "=== Searching for 'ws' imports in code/core ==="
rg -n "ws" code/core/ --type ts --type tsx --type js --type jsx 2>/dev/null | grep -i "import\|require\|from" | grep -v "ws:" | head -20Length of output: 153
🏁 Script executed:
#!/bin/bash
# Check package.json exports and main fields
echo "=== Checking package.json main entry points ==="
jq '.main, .exports, .types' code/core/package.json
echo ""
echo "=== Checking if build output exists ==="
ls -la code/core/dist/ 2>/dev/null | head -20 || echo "dist/ not found"
echo ""
echo "=== Looking for tsconfig or build config ==="
find code/core/ -maxdepth 1 -type f \( -name "tsconfig*" -o -name "esbuild*" -o -name "build*" \) 2>/dev/nullLength of output: 4478
🏁 Script executed:
#!/bin/bash
# Verify build-config.ts uses the packages as build tools, not runtime
echo "=== Checking build-config.ts for package usage ==="
grep -n "recast\|semver\|ws" code/core/build-config.ts
echo ""
echo "=== Checking for dynamic requires or imports ==="
rg -n "require\s*\(\s*['\"]" code/core/src/ --type ts --type tsx | grep -E "recast|semver|ws" | head -10
echo ""
echo "=== Checking csf-tools and csf for recast usage (CSF parsing tool) ==="
rg -n "recast" code/core/src/csf-tools/ --type ts --type tsx 2>/dev/null || echo "No recast in csf-tools"
rg -n "recast" code/core/src/csf/ --type ts --type tsx 2>/dev/null || echo "No recast in csf"Length of output: 258
🏁 Script executed:
#!/bin/bash
# Broader search for package usage across entire code/core
echo "=== Searching entire code/core for 'recast' (not just src/) ==="
rg -n "recast" code/core/ --type ts --type js 2>/dev/null | head -20
echo ""
echo "=== Searching entire code/core for 'semver' (not just src/) ==="
rg -n "semver" code/core/ --type ts --type js 2>/dev/null | head -20
echo ""
echo "=== Searching entire code/core for 'ws' (not just src/) ==="
rg -n "\bws\b" code/core/ --type ts --type js 2>/dev/null | grep -v "ws:" | head -20
echo ""
echo "=== Checking scripts directory ==="
ls -la code/core/scripts/
echo ""
echo "=== Checking if these are transitive deps (searching package.json dependencies) ==="
jq '.dependencies | keys[] as $k | select($k | contains("recast") or contains("semver") or contains("ws"))' code/core/package.jsonLength of output: 5018
🏁 Script executed:
#!/bin/bash
# Check what packages depend on recast, semver, ws
echo "=== Dependencies that might use recast/semver/ws ==="
jq '.dependencies | keys[]' code/core/package.json | xargs -I {} sh -c 'echo "Checking {}..."; jq ".devDependencies.{} // .dependencies.{}" code/core/package.json' 2>/dev/null | head -30
echo ""
echo "=== Direct dependency entries for moved packages ==="
jq '.dependencies.recast, .dependencies.semver, .dependencies.ws, .devDependencies.recast, .devDependencies.semver, .devDependencies.ws' code/core/package.jsonLength of output: 472
Do NOT merge—runtime imports found in bundled code.
Critical verification result: All three packages are actively imported in runtime code that gets exported and bundled:
- recast: Used in code/core/src/csf-tools/CsfFile.ts, code/core/src/babel/index.ts, code/core/src/cli/eslintPlugin.ts for AST parsing and code generation
- semver: Used in code/core/src/manager-api/modules/versions.ts, code/core/src/cli/helpers.ts, code/core/src/cli/detect.ts for version comparisons
- ws: Used in code/core/src/core-server/utils/get-server-channel.ts for WebSocket server communication
These modules are exported via package.json's exports field and will be bundled into dist/. Moving these packages to devDependencies without a verified build system fix will break runtime functionality.
Ensure a corresponding build system change exists that prevents re-bundling of these packages, or keep them in dependencies if they must be available at runtime.
|
View your CI Pipeline Execution ↗ for commit 6432934
☁️ Nx Cloud last updated this comment at |
|
Oh goody, |
1 participant
storybookbundles dependencies it still requires (e.g.@testing-library/jest-dom) #32822What I did
Move
ws,recastandsemverfromdependenciestodevDependencies.These three dependencies should have been fully external. However, there appears to be no need for them to be external, and owing to some bug in the build system they were actually being bundled (for runtime) anyway, which only their types being external.
This PR moves them to
devDependencieswhere they probably belong.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 PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit