Repository navigation
Typescript: Exclude all dunderscore prefixed folders from production compilation - #36740
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe declaration generator now uses the ChangesDeclaration type exclusions
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
|
It works! If there's a chance this could be backported to 10 that would be phenomenal. |
There was a problem hiding this comment.
🤖 I am the Storybook contribution review bot; a human maintainer approved this message before it was posted.
Review phase: Runtime verification (stage 3 of 3)We are trying a three-stage automated review: architecture, code quality, and runtime verification. This message states which stages ran and which were skipped. A human maintainer approves each public message and makes the final decision. Please tell us whether the feedback and evidence are useful.
Hi @mrginglymus, thanks for tracing this leak back to the __for-testing__ fixture. That was a tricky one to find! All three review stages passed on commit 16f00d6: architecture, code quality and runtime verification. No stage was skipped.
Runtime verification. We ran a local production build of the storybook package (scripts/build/build-package.ts --prod, default rolldown-tsgo dts path) at the base commit 271a436 and at this PR's head:
- Base:
dist/**/*.d.tscontains 165import("@storybook/react")type references (162 ×Args, 3 ×Parameters), for example in theenhanceArgTypessignature. This is the leak from #35663, and a second clean build gave the same count. - Head: the same build emits 0 of them. Those types now resolve through
storybook/internal/typesandstorybook/internal/csf. - No production types dropped: the d.ts file list (61 files), the set of emitted source modules (717) and the exported names of every entry file are identical at base and head. Every other changed line is one of these type-reference rewrites.
Architecture and code quality. Every __* directory under code/ holds only tests, mocks or fixtures, and a tsconfig exclude only drops files that no production module imports. So **/__*/** follows the repo convention and cannot remove a type that is in use. It is also simpler than keeping a list of directory names up to date.
One optional suggestion: a short comment on that line, for example // __tests__, __mocks__, __testfixtures__, …: test-only by convention, would record the convention the exclude now depends on. This is not required.
Scope of this check: we verified only the storybook package, which is the one named in #35663. We did not type-check the emitted d.ts in a consumer project. A maintainer makes the final decision on merging this PR and on your request for a 10.x backport.
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
Closes #35663
What I did
Production builds first transpile code to plain
.d.tsfiles. There is an exclusion filter on this process to exclude test files, but it did not capture the file contianing this line:storybook/code/core/src/core-server/__for-testing__/main.ts
Line 4 in 271a436
This transiently imports
@storybook/reactwhich reexportsArgsfromstorybook/internal/types. If this was found first(?) typescript would deem it the canonical import source and reference it in all usages.I updated the exclusion glob to exclude any dunderscore prefixed directory.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
storybookcode contains no type references to@storybook/react.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.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake 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 - 🚢 Released
8dbf69eThis pull request has been released as canary packages. Try it out in a new project or update an existing project with the commands below.