CLI: Switch over to modern-tar#32763
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughThe pull request replaces a tarball download helper with a streaming-based extraction pipeline: removes the Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI code
participant Fetch as Fetch API
participant Stream as Gunzip/Streams
participant Tar as modern-tar.unpackTar
rect rgb(230,240,255)
Note over CLI,Fetch: Streaming extraction flow
CLI->>Fetch: fetch(tarballUrl)
Fetch-->>CLI: Response
alt response missing or !ok
CLI-->>CLI: throw Error
else ok
CLI->>Stream: response.body (ReadableStream)
Stream->>Tar: piped/gunzip stream
Tar-->>CLI: extracted files
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
🧹 Nitpick comments (2)
code/core/src/cli/dirs.ts (2)
33-36: Enhance error message with HTTP status details.The error handling checks the response but doesn't include the HTTP status code or reason, making debugging more difficult when tarball downloads fail.
Apply this diff to include the status in the error message:
const response = await fetch(url); if (!response.ok || !response.body) { - throw new Error(`Failed to download tarball from ${url}`); + throw new Error(`Failed to download tarball from ${url}: ${response.status} ${response.statusText}`); }
39-43: Consider using thestripoption to eliminate the extra directory join.The tarball extracts with a
package/root directory (as evidenced by line 45's join), but modern-tar'sunpackTarsupports astripoption that removes leading directory components during extraction.Apply this diff to simplify the extraction:
await pipeline( Readable.fromWeb(response.body as ReadableStream<Uint8Array>), createGunzip(), - unpackTar(tempDirectory) + unpackTar(tempDirectory, { strip: 1 }) );Then update line 45:
-return join(tempDirectory, 'package'); +return tempDirectory;Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
code/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
code/core/package.json(1 hunks)code/core/src/cli/dirs.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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.jsoncode/core/src/cli/dirs.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/cli/dirs.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/cli/dirs.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/cli/dirs.ts
🔇 Additional comments (3)
code/core/src/cli/dirs.ts (2)
2-5: LGTM: Appropriate imports for streaming-based extraction.The imports correctly set up the streaming pipeline: Node streams for interop, zlib for decompression, and modern-tar for extraction.
Also applies to: 12-12
39-43: No issues found. The directory is already created beforeunpackTaris called.The
tempDirectoryon line 19 is obtained fromawait temporaryDirectory(), which explicitly creates the directory viamkdirSync()in its implementation (code/core/src/common/utils/cli.ts:21) before returning it. The created directory is then passed tounpackTaron line 42, so the concern about missing directory creation is already addressed.Likely an incorrect or invalid review comment.
code/core/package.json (1)
292-292: The original review comment is incorrect.The npm registry verification confirms that version 0.5.4 of modern-tar exists and is the current latest release. The package specification
"modern-tar": "^0.5.4"is valid and will install successfully. The earlier learnings stating the latest stable was 0.4.0 were inaccurate.Likely an incorrect or invalid review comment.
vanessayuenn
changed the title
vanessayuenn
changed the title
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 43 | 43 | 0 |
| Self size | 24.84 MB | 24.15 MB | 🎉 -690 KB 🎉 |
| Dependency size | 17.36 MB | 17.36 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 187 | 0 |
| Self size | 928 KB | 928 KB | 0 B |
| Dependency size | 74.79 MB | 74.10 MB | 🎉 -690 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 169 | 169 | 0 |
| Self size | 35 KB | 35 KB | 0 B |
| Dependency size | 71.22 MB | 70.53 MB | 🎉 -690 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 44 | 44 | 0 |
| Self size | 1.55 MB | 1.55 MB | 🎉 -60 B 🎉 |
| Dependency size | 42.20 MB | 41.51 MB | 🎉 -690 KB 🎉 |
| Bundle Size Analyzer | node | node |
|
View your CI Pipeline Execution ↗ for commit 6f2a421
☁️ Nx Cloud last updated this comment at |
Latest merge commit triggered these failures. Seems to be unrelated to this maybe? |
|
yes, unrelated, we're working on it! |
4 participants
Related: #29038 cc @JReinhold
What I did
If there is any appetite to reduce the dependency count, I'd love to suggest using modern-tar that solely relies on native APIs to cut down the dependency tree and reduce the bundle size by around ~2MB. It also replaces
gunzip-maybewith justnode:zlib.This has a very small surface area which makes me believe this is a safe migration to undertake. Please let me know if there any concerns! We also recently switched
@bluwy/giget-corewhich is a very similar mechanism forcreate-astro!Ref: https://npmgraph.js.org/?q=storybook#deps=devDependencies&select=exact%3A%40ndelangen%2Fget-tarball%403.0.9
Checklist for Contributors
Testing
This code should only affect the
create-storybookpackage and many test helpers.The changes in this PR are covered in the following automated tests:
Manual testing
create-storybook/dist/bin/index.jsdirectly.yarn task --task sandbox --start-from auto --template react-vite/default-tsDocumentation
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>Depending on the discretion of the Storybook team, I think a canary release to properly test
npx create-storybookwould be the most safe approach to testing this E2E sincenpxbehaves slightly different than running the.jsfile directly. But this is an optional suggestion since the scope of this PR is very small.Summary by CodeRabbit