fix: Update TagOptions to allow undefined defaultFilterSelection and add tests for TagsOptions#33895
fix: Update TagOptions to allow undefined defaultFilterSelection and add tests for TagsOptions#33895unional wants to merge 1 commit intostorybookjs:nextfrom
Conversation
…add tests for TagsOptions
📝 WalkthroughWalkthroughModifies the TagOptions type definition to allow undefined values for the defaultFilterSelection property, introduces a test file validating the behavior of defaultFilterSelection in TagsOptions when NODE_ENV differs, and comments out a TypeScript compiler option in the configuration. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
✨ Finishing Touches
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/types/modules/core-common.test.ts (1)
7-19: Consider simplifying the test to directly useundefined.The test is named "accepts undefined" but the conditional logic means
undefinedis only assigned whenNODE_ENV === 'development'. For a type-level test, directly assigningundefinedwould be clearer and more deterministic:♻️ Suggested simplification
it('accepts undefined', () => { const config: StorybookConfig = { stories: [], framework: '@storybook/react-vite', tags: { unit: { - defaultFilterSelection: - process.env['NODE_ENV'] !== 'development' ? 'exclude' : undefined, + defaultFilterSelection: undefined, }, }, }; expect(config).toBeDefined(); });If you want to demonstrate the conditional pattern from the PR description works, consider adding a separate test case:
it('accepts conditional assignment pattern', () => { const isDev = process.env['NODE_ENV'] === 'development'; const config: StorybookConfig = { stories: [], framework: '@storybook/react-vite', tags: { unit: { defaultFilterSelection: isDev ? undefined : 'exclude', }, }, }; expect(config).toBeDefined(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/types/modules/core-common.test.ts` around lines 7 - 19, The test named "accepts undefined" should deterministically assign undefined rather than depending on NODE_ENV; update the test that constructs a StorybookConfig to set tags.unit.defaultFilterSelection: undefined directly (referencing the StorybookConfig type and the defaultFilterSelection property) so the type-level acceptance is clear, and optionally add a separate test that demonstrates the conditional pattern by using a local isDev boolean and assigning defaultFilterSelection: isDev ? undefined : 'exclude' to validate both cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/types/modules/core-common.test.ts`:
- Around line 7-19: The test named "accepts undefined" should deterministically
assign undefined rather than depending on NODE_ENV; update the test that
constructs a StorybookConfig to set tags.unit.defaultFilterSelection: undefined
directly (referencing the StorybookConfig type and the defaultFilterSelection
property) so the type-level acceptance is clear, and optionally add a separate
test that demonstrates the conditional pattern by using a local isDev boolean
and assigning defaultFilterSelection: isDev ? undefined : 'exclude' to validate
both cases.
What I did
Add
undefinedto the defaultFilterSelection to allow simplier usage:This is generally related to supporting
exactOptionalPropertyTypes.But instead of fixing all issue which will be a lot more substantial,
this PR just focus on this fix.
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!
Can be validated with mutation test by uncommenting the tsconfig line and observe the error in the test (you may need to restart the TS server to see).
Can't enable that directly as there are other failures.
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
Tests
Chores