-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
CSF: Fix play-fn tag for methods
#32695
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
Conversation
play-fn tag for methods
📝 WalkthroughWalkthroughParser now preserves object-method AST nodes for meta and story object properties (e.g., play), treats object-methods as mountable, records those method nodes in _metaAnnotations/_storyAnnotations, and updates __stats (including mount detection). New tests validate play/mount detection across story and meta object exports. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SRC as CSF Source
participant Parser as CsfFile Parser
participant Meta as _metaAnnotations
participant Stories as _storyAnnotations
participant Out as Parsed Result
SRC->>Parser: parse(CSF file)
alt default export is object (meta)
Parser->>Meta: iterate meta properties
alt property is object-method (e.g., play)
Meta-->>Parser: preserve method AST node
Note right of Parser: tag meta with "play-fn" and store node
else
Meta-->>Parser: store value as before
end
end
loop each story export
Parser->>Stories: inspect story object property
alt property is object-method (e.g., play)
Stories-->>Parser: preserve method AST node under story key
Note right of Parser: tag story "play-fn", set __stats.play = true
alt method signature includes mount param
Note right of Parser: set __stats.mount = true
end
else
Stories-->>Parser: store value as before
end
end
Parser->>Out: assemble annotations, tags, and __stats
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (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:
🧬 Code graph analysis (1)code/core/src/csf-tools/CsfFile.test.ts (1)
⏰ 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 (4)
Comment |
|
View your CI Pipeline Execution ↗ for commit c89f490
☁️ 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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/csf-tools/CsfFile.ts (1)
602-629: Good refactoring to support object method syntax.The code now correctly handles play functions defined with method syntax (
play() {}) by storing the method node, enabling proper play-fn tag detection.Consider extending mount detection to object methods.
The
hasMountfunction (lines 177-192) only checks for arrow functions and function declarations, not object methods. Object methods with amountparameter (e.g.,play({ mount }) {}) won't be detected. This appears intentional based on test expectations, but consider if this is a limitation worth addressing in the future.To support mount detection for object methods, update the
hasMountfunction:const hasMount = (play: t.Node | undefined) => { - if (t.isArrowFunctionExpression(play) || t.isFunctionDeclaration(play)) { + if (t.isArrowFunctionExpression(play) || t.isFunctionDeclaration(play) || t.isObjectMethod(play)) { const params = play.params; if (params.length >= 1) { const [arg] = params;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/csf-tools/CsfFile.test.ts(1 hunks)code/core/src/csf-tools/CsfFile.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/csf-tools/CsfFile.tscode/core/src/csf-tools/CsfFile.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/csf-tools/CsfFile.tscode/core/src/csf-tools/CsfFile.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/csf-tools/CsfFile.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/csf-tools/CsfFile.test.ts
🧬 Code graph analysis (1)
code/core/src/csf-tools/CsfFile.test.ts (1)
code/core/src/csf-tools/CsfFile.ts (1)
parse(416-931)
⏰ 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). (3)
- GitHub Check: Danger JS
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/core/src/csf-tools/CsfFile.ts (1)
351-352: LGTM! Correctly handles object methods in meta.The change properly distinguishes between object methods (e.g.,
play() {}) and regular properties, storing the appropriate node for later play-fn tag detection.code/core/src/csf-tools/CsfFile.test.ts (2)
1791-1821: LGTM! Good test coverage for story-level play methods.The test correctly verifies that object method syntax (
play({ context }) {}) receives theplay-fntag and appropriate__statsvalues.
1823-1850: LGTM! Good test coverage for meta-level play methods.The test correctly verifies that object method syntax in meta (
play({ context }) {}) adds theplay-fntag to meta and propagates play detection to stories.
5d1c0c5 to
c5ee382
Compare
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/csf-tools/CsfFile.ts (1)
602-630: Approve with recommended enhancement for hasMount compatibility.The main fix correctly handles ObjectMethod nodes for story annotations, enabling proper detection of play methods defined with method syntax.
Consider enhancing the
hasMountfunction (lines 177-192) to handle ObjectMethod nodes. Currently it only checks for ArrowFunctionExpression and FunctionDeclaration:const hasMount = (play: t.Node | undefined) => { - if (t.isArrowFunctionExpression(play) || t.isFunctionDeclaration(play)) { + if (t.isArrowFunctionExpression(play) || t.isFunctionDeclaration(play) || t.isObjectMethod(play)) { const params = play.params;This would allow mount detection to work correctly for play methods like:
play({ mount }) { /* ... */ }Similarly, if render methods become common, consider handling ObjectMethod for the
__isArgsStorydetection at line 607 to correctly identify render methods with no parameters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/csf-tools/CsfFile.test.ts(1 hunks)code/core/src/csf-tools/CsfFile.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/csf-tools/CsfFile.tscode/core/src/csf-tools/CsfFile.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/csf-tools/CsfFile.tscode/core/src/csf-tools/CsfFile.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/csf-tools/CsfFile.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/csf-tools/CsfFile.test.ts
🧬 Code graph analysis (1)
code/core/src/csf-tools/CsfFile.test.ts (1)
code/core/src/csf-tools/CsfFile.ts (1)
parse(416-932)
⏰ 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)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/core/src/csf-tools/CsfFile.test.ts (2)
1791-1821: LGTM! Correctly tests object method play function detection.The test verifies that a play function defined using object method syntax is properly detected and tagged with
play-fn, which aligns with the PR objectives.
1823-1850: LGTM! Correctly tests meta-level play method detection.The test verifies that a play function defined at the meta level using object method syntax is properly detected, tagged, and inherited by stories.
code/core/src/csf-tools/CsfFile.ts (1)
351-352: LGTM! Correctly stores ObjectMethod nodes for meta annotations.The change properly distinguishes between ObjectMethod (method shorthand) and ObjectProperty (property with value), storing the appropriate node type in each case. This enables correct detection of play methods defined at the meta level.
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/csf-tools/CsfFile.ts (1)
604-636: Consider refining the type cast for accuracy.The cast
as t.ObjectProperty[]at line 604 is slightly inaccurate since the properties array can contain botht.ObjectPropertyandt.ObjectMethodnodes. While the runtime logic correctly handles both types through thet.isObjectMethod(p)check, the cast may cause confusion.Consider this refactor for clarity:
// CSF3 object export - (storyNode.properties as t.ObjectProperty[]).forEach((p) => { + storyNode.properties.forEach((p) => { + if (!t.isObjectProperty(p) && !t.isObjectMethod(p)) { + return; + }Alternatively, if type narrowing is needed:
- (storyNode.properties as t.ObjectProperty[]).forEach((p) => { + (storyNode.properties as Array<t.ObjectProperty | t.ObjectMethod>).forEach((p) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/csf-tools/CsfFile.test.ts(2 hunks)code/core/src/csf-tools/CsfFile.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/csf-tools/CsfFile.tscode/core/src/csf-tools/CsfFile.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/csf-tools/CsfFile.tscode/core/src/csf-tools/CsfFile.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/csf-tools/CsfFile.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/csf-tools/CsfFile.test.ts
🧬 Code graph analysis (1)
code/core/src/csf-tools/CsfFile.test.ts (1)
code/core/src/csf-tools/CsfFile.ts (1)
parse(420-936)
⏰ 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)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (5)
code/core/src/csf-tools/CsfFile.ts (2)
177-196: LGTM! Mount detection now supports method syntax.The addition of
t.isObjectMethod(play)correctly extends mount detection to handle play functions defined with method shorthand (e.g.,play({ mount }) {}).
353-356: LGTM! Meta annotations now preserve method nodes.Storing the entire method node (when
t.isObjectMethod(p)) instead of just the value enables proper detection of method-syntax properties in meta configuration.code/core/src/csf-tools/CsfFile.test.ts (3)
1791-1821: LGTM! Test validates story-level play method detection.This test correctly verifies that play functions defined using method shorthand syntax are detected and tagged with
play-fn.
1823-1850: LGTM! Test validates meta-level play method detection.This test ensures that play methods defined in the meta configuration are correctly detected and propagate the
play-fntag to both meta and stories.
1916-1946: LGTM! Test validates mount detection in method syntax.This test confirms that the
hasMountfunction correctly detects mount usage when play is defined using method shorthand syntax with destructured parameters.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 187 | 0 |
| Self size | 68 KB | 68 KB | 0 B |
| Dependency size | 31.88 MB | 31.89 MB | 🚨 +10 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/angular
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 187 | 0 |
| Self size | 126 KB | 126 KB | 0 B |
| Dependency size | 30.01 MB | 30.02 MB | 🚨 +10 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 191 | 191 | 0 |
| Self size | 17 KB | 17 KB | 🚨 +24 B 🚨 |
| Dependency size | 28.59 MB | 28.61 MB | 🚨 +10 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 532 | 532 | 0 |
| Self size | 950 KB | 950 KB | 0 B |
| Dependency size | 58.59 MB | 58.60 MB | 🚨 +10 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.55 MB | 43.56 MB | 🚨 +10 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/server-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 199 | 199 | 0 |
| Self size | 17 KB | 17 KB | 🎉 -54 B 🎉 |
| Dependency size | 33.13 MB | 33.14 MB | 🚨 +10 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 187 | 0 |
| Self size | 927 KB | 927 KB | 0 B |
| Dependency size | 79.96 MB | 79.97 MB | 🚨 +11 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 | 76.39 MB | 76.40 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 170 | 170 | 0 |
| Self size | 21 KB | 21 KB | 0 B |
| Dependency size | 31.04 MB | 31.05 MB | 🚨 +10 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
CSF: Fix `play-fn` tag for methods (cherry picked from commit b51732d)
CSF: Fix `play-fn` tag for methods (cherry picked from commit b51732d)
Closes #32687
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
🦋 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
Improvements
Bug Fixes
Tests