Core: Improve es-toolkit usage for better tree-shaking#32787
Conversation
📝 WalkthroughWalkthroughSeveral files switch from importing utilities as named exports from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (7)code/**/*.{test,spec}.{ts,tsx}📄 CodeRabbit inference engine (.cursorrules)
Files:
**/*.test.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
Files:
**/*.{js,jsx,json,html,ts,tsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.@(test|spec).{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
code/**/*.{ts,tsx,js,jsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (8)📚 Learning: 2025-09-17T08:11:47.196ZApplied to files:
📚 Learning: 2025-09-17T08:11:47.196ZApplied to files:
📚 Learning: 2025-10-13T13:33:14.659ZApplied to files:
📚 Learning: 2025-09-17T08:11:47.196ZApplied to files:
📚 Learning: 2025-09-17T08:11:47.196ZApplied to files:
📚 Learning: 2025-09-17T08:11:47.196ZApplied to files:
📚 Learning: 2025-09-17T08:11:47.197ZApplied to files:
📚 Learning: 2025-09-17T08:11:47.197ZApplied to files:
⏰ 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). (2)
🔇 Additional comments (2)
Comment |
|
View your CI Pipeline Execution ↗ for commit a057075
☁️ Nx Cloud last updated this comment at |
mrginglymus
force-pushed
the
es-toolkit-ohno
branch
from
82889b7 to
14be8b5
Compare
![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 (1)
code/core/src/core-server/utils/stories-json.test.ts (1)
19-19: Mock path correctly updated.The mock path has been properly updated to match the new import.
Consider adding
spy: trueoption to align with testing guidelines:-vi.mock('es-toolkit/compat/debounce'); +vi.mock('es-toolkit/compat/debounce', { spy: true });Note: This applies to all mocks in the file (lines 18, 20, 22) as per coding guidelines.
As per coding guidelines
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx(1 hunks)code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx(1 hunks)code/addons/docs/src/blocks/controls/Color.tsx(1 hunks)code/addons/docs/src/blocks/controls/Object.tsx(1 hunks)code/core/src/core-server/utils/stories-json.test.ts(3 hunks)code/core/src/core-server/utils/stories-json.ts(1 hunks)code/core/src/manager/components/sidebar/useLastViewed.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- code/core/src/manager/components/sidebar/useLastViewed.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
- code/addons/docs/src/blocks/controls/Object.tsx
- code/addons/docs/src/blocks/controls/Color.tsx
- code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx
🧰 Additional context used
📓 Path-based instructions (7)
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/core/src/core-server/utils/stories-json.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access mocked functions
Implement mock behaviors in beforeEach blocks
Mock all required dependencies that the test subject uses
Mock implementations should be placed in beforeEach blocks
Each mock implementation should return a Promise for async functions
Mock implementations should match the expected return type of the original function
Use vi.mocked() to access and implement mock behaviors
Mock all required properties and methods that the test subject uses
Avoid direct function mocking without vi.mocked()
Avoid mock implementations outside of beforeEach blocks
Avoid mocking without the spy: true option
Avoid inline mock implementations within test cases
Avoid mocking only a subset of required dependencies
Mock at the highest level of abstraction needed
Keep mock implementations simple and focused
Use type-safe mocking with vi.mocked()
Document complex mock behaviors
Group related mocks together
Files:
code/core/src/core-server/utils/stories-json.test.ts
**/*.{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/src/core-server/utils/stories-json.test.tscode/core/src/core-server/utils/stories-json.ts
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/core/src/core-server/utils/stories-json.test.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/core-server/utils/stories-json.test.tscode/core/src/core-server/utils/stories-json.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/core-server/utils/stories-json.test.tscode/core/src/core-server/utils/stories-json.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/core-server/utils/stories-json.test.tscode/core/src/core-server/utils/stories-json.ts
🧠 Learnings (2)
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to **/*.@(test|spec).{ts,tsx,js,jsx} : Mock external dependencies in tests using `vi.mock()` (e.g., filesystem, loggers)
Applied to files:
code/core/src/core-server/utils/stories-json.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mocked() to access and implement mock behaviors
Applied to files:
code/core/src/core-server/utils/stories-json.test.ts
⏰ 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 (3)
code/core/src/core-server/utils/stories-json.ts (1)
7-7: LGTM! Import path change supports tree-shaking.The change from a named import from the barrel module to a default import from the specific module path aligns with the PR objective and should improve tree-shaking.
code/core/src/core-server/utils/stories-json.test.ts (2)
8-8: LGTM! Test import matches source file.The import change is consistent with the source file and correctly uses the default import syntax.
577-579: LGTM! Correct handling of default export in actual import.The change correctly accesses
.defaultfrom the imported module to get the actual debounce implementation, which is necessary since the module now uses a default export.
mrginglymus
force-pushed
the
es-toolkit-ohno
branch
from
14be8b5 to
4b99908
Compare
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 (7)
code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx(1 hunks)code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx(1 hunks)code/addons/docs/src/blocks/controls/Color.tsx(1 hunks)code/addons/docs/src/blocks/controls/Object.tsx(1 hunks)code/core/src/core-server/utils/stories-json.test.ts(3 hunks)code/core/src/core-server/utils/stories-json.ts(1 hunks)code/core/src/manager/components/sidebar/useLastViewed.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- code/addons/docs/src/blocks/controls/Color.tsx
- code/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsx
- code/core/src/manager/components/sidebar/useLastViewed.ts
- code/addons/docs/src/blocks/components/ArgsTable/ArgValue.tsx
- code/addons/docs/src/blocks/controls/Object.tsx
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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/src/core-server/utils/stories-json.tscode/core/src/core-server/utils/stories-json.test.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/core-server/utils/stories-json.tscode/core/src/core-server/utils/stories-json.test.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/core-server/utils/stories-json.tscode/core/src/core-server/utils/stories-json.test.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/core-server/utils/stories-json.tscode/core/src/core-server/utils/stories-json.test.ts
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/core/src/core-server/utils/stories-json.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access mocked functions
Implement mock behaviors in beforeEach blocks
Mock all required dependencies that the test subject uses
Mock implementations should be placed in beforeEach blocks
Each mock implementation should return a Promise for async functions
Mock implementations should match the expected return type of the original function
Use vi.mocked() to access and implement mock behaviors
Mock all required properties and methods that the test subject uses
Avoid direct function mocking without vi.mocked()
Avoid mock implementations outside of beforeEach blocks
Avoid mocking without the spy: true option
Avoid inline mock implementations within test cases
Avoid mocking only a subset of required dependencies
Mock at the highest level of abstraction needed
Keep mock implementations simple and focused
Use type-safe mocking with vi.mocked()
Document complex mock behaviors
Group related mocks together
Files:
code/core/src/core-server/utils/stories-json.test.ts
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/core/src/core-server/utils/stories-json.test.ts
🧠 Learnings (2)
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to **/*.@(test|spec).{ts,tsx,js,jsx} : Mock external dependencies in tests using `vi.mock()` (e.g., filesystem, loggers)
Applied to files:
code/core/src/core-server/utils/stories-json.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mocked() to access and implement mock behaviors
Applied to files:
code/core/src/core-server/utils/stories-json.test.ts
⏰ 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 (3)
code/core/src/core-server/utils/stories-json.ts (1)
7-7: LGTM! Import path optimized for tree-shaking.The change from a named import of the entire compat module to a default import from the specific debounce submodule improves tree-shaking as intended. The usage remains compatible.
code/core/src/core-server/utils/stories-json.test.ts (2)
8-8: LGTM! Test import updated consistently.The import correctly mirrors the source file change to use the default export from the specific debounce submodule path.
577-579: LGTM! Correct default export access pattern.The
importActualstatement correctly accesses the.defaultexport from the new module path, and the type annotation is properly updated to match.
|
|
||
| vi.mock('watchpack'); | ||
| vi.mock('es-toolkit/compat'); | ||
| vi.mock('es-toolkit/compat/debounce'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add spy: true option to the mock.
Per coding guidelines, all vi.mock() calls should include the spy: true option for package and file mocks.
Apply this diff:
-vi.mock('es-toolkit/compat/debounce');
+vi.mock('es-toolkit/compat/debounce', { spy: true });As per coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vi.mock('es-toolkit/compat/debounce'); | |
| vi.mock('es-toolkit/compat/debounce', { spy: true }); |
🤖 Prompt for AI Agents
In code/core/src/core-server/utils/stories-json.test.ts around line 19, the
vi.mock('es-toolkit/compat/debounce') call is missing the required spy option;
update the vi.mock invocation to pass the spy: true option (preserving any
factory callback if present) so the mock is created with spying enabled per
coding guidelines.
mrginglymus
force-pushed
the
es-toolkit-ohno
branch
from
4b99908 to
d37b8bd
Compare
yannbf
changed the title
es-toolkit/compat imports as they cannot be effectively shaken
|
This is the 'minimum viable change' - I suspect we could switch to the non-compat flavours of each without issue which might trim even more off. |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 18 | 18 | 0 |
| Self size | 2.07 MB | 1.92 MB | 🎉 -153 KB 🎉 |
| Dependency size | 10.21 MB | 10.21 MB | 🚨 +12 B 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 43 | 43 | 0 |
| Self size | 24.15 MB | 23.62 MB | 🎉 -531 KB 🎉 |
| Dependency size | 17.36 MB | 17.36 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 532 | 532 | 0 |
| Self size | 950 KB | 751 KB | 🎉 -200 KB 🎉 |
| Dependency size | 58.77 MB | 58.57 MB | 🎉 -200 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 124 | 124 | 0 |
| Self size | 4.10 MB | 3.90 MB | 🎉 -200 KB 🎉 |
| Dependency size | 21.81 MB | 21.61 MB | 🎉 -200 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 157 | 157 | 0 |
| Self size | 31 KB | 31 KB | 🎉 -24 B 🎉 |
| Dependency size | 23.19 MB | 22.99 MB | 🎉 -200 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 114 | 114 | 0 |
| Self size | 37 KB | 37 KB | 0 B |
| Dependency size | 19.75 MB | 19.55 MB | 🎉 -200 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 272 | 272 | 0 |
| Self size | 25 KB | 25 KB | 0 B |
| Dependency size | 43.72 MB | 43.52 MB | 🎉 -200 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 187 | 0 |
| Self size | 928 KB | 928 KB | 🚨 +84 B 🚨 |
| Dependency size | 74.10 MB | 73.57 MB | 🎉 -531 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 169 | 169 | 0 |
| Self size | 35 KB | 35 KB | 🚨 +42 B 🚨 |
| Dependency size | 70.53 MB | 70.00 MB | 🎉 -531 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 44 | 44 | 0 |
| Self size | 1.55 MB | 1.55 MB | 0 B |
| Dependency size | 41.51 MB | 40.98 MB | 🎉 -531 KB 🎉 |
| Bundle Size Analyzer | node | node |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 2 | 2 | 0 |
| Self size | 894 KB | 695 KB | 🎉 -200 KB 🎉 |
| Dependency size | 28 KB | 28 KB | 🎉 -6 B 🎉 |
| Bundle Size Analyzer | Link | Link |
3 participants
What I did
Remove imports from
es-toolkit/compatbarrel file asesbuildwill not tree shake it.@storybook/react - browser -- before
after
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
Refactor
Tests