-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Sandboxes: Typecheck check production storybook packages #32447
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
We were pinning to a version that did not support a feature we were setting
|
View your CI Pipeline Execution ↗ for commit 816d794
☁️ Nx Cloud last updated this comment at |
|
Well there's a thing. Switching the sandbox to pnp masks the mdx type issue...somehow. It also throws up a lot of other issues, as it's a shortcut to finding packages importing transitive dependencies. |
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.
15 files reviewed, 3 comments
| // Lots of unnecessary imports of react that need fixing | ||
| tsConfigJson.compilerOptions.noUnusedLocals = false; | ||
| // Means we can check our own public types | ||
| tsConfigJson.compilerOptions.skipLibCheck = false; |
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.
style: Setting skipLibCheck to false may cause type errors from third-party packages. Consider documenting why this is necessary or if it should be conditional
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.
shush greptile, that's the point! you managed to get that in your summary
also if there are type errors in the third-party packages we should be fixing them
scripts/tasks/check-sandbox.ts
Outdated
| } | ||
|
|
||
| export const checkSandbox: Task = { | ||
| description: 'Typecheck the a sandbox', |
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.
syntax: Typo: 'Typecheck the a sandbox' should be 'Typecheck a sandbox' or 'Typecheck the sandbox'
| description: 'Typecheck the a sandbox', | |
| description: 'Typecheck a sandbox', |
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.
this one I will give you
|
|
||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment -- we can't expect error as it isn't an error in development but it is in sandbox | ||
| // @ts-ignore unused props | ||
| export const Text: React.FC<Props> = ({ padding = '0', margin }) => <>Text</>; |
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.
style: Consider using underscore prefix for unused parameters (_padding, _margin) as a TypeScript convention instead of ts-ignore, which would be more explicit about intentional non-usage
| export const Text: React.FC<Props> = ({ padding = '0', margin }) => <>Text</>; | |
| export const Text: React.FC<Props> = ({ padding: _padding = '0', margin: _margin }) => <>Text</>; |
4c28d88 to
87ff06b
Compare
|
@ndelangen task failed successfully, at last: This job won't pass until #32442 is in. |
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
| skipTasks?: SkippableTask[]; | ||
| /** | ||
| * Should the sandbox be type checked after build. Not part of skipTasks as the default answer | ||
| * will be 'no', at least initially | ||
| */ | ||
| typeCheck?: boolean; |
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.
This could have been implemented with the skipTask array. Do you think that that is an easy adjustment to make, or difficult? If it's hard, I'm okay with it as-is.
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.
Now that I understand how to use get-template, not that hard, just a lot of keyboard work.
The benefit is that, currently at least, adding type checking means conditionally modifying the sandbox slightly.
storybook/scripts/tasks/sandbox-parts.ts
Lines 223 to 225 in 4051541
| if (template.typeCheck) { | |
| await prepareTypeChecking(cwd); | |
| } |
There are a couple of options here, if we switch to skip-tasks:
- replace that with a
if (!skipTasks.includes('type-check')) - Update the sandbox definition to include this type checking
|
@mrginglymus Can you take another look at this PR? 1 thing I missed previously, is that the CircleCI config is actually generated based on a src and a circleci command, you can read about it in the readme in the .circleci folder. |
|
@ndelangen updated. latest failures appears to be caused (ish) by #32822 - |
|
@ndelangen all green again! I guess sb10 is pulling in more test deps that are missing (upstream) their associated types. |
Closes #31554
What I did
Initial work pushed for visibility and assistance
Pinned typescript
We are pinning sandboxes to an older version of typescript than expected by
create vite.Solution: remove pin. I suspect a lot of the other pins can be removed as they seem vestigial.
Stricter tsconfig
The tsconfig created by
create viteis stricter than the one used by storybook.We have a lot of unused
import React from 'react';that isn't necessary when you havejsx: "react-jsx".codetsconfig hasjsx: "preserve", which seems wrong.The
create vitetsconfig also haserasableSyntaxOnlyset totrue, which disallowsenumusage, but we have some in our examples.Solution: reduce strictness of tsconfig, though ideally we'd fix the issue with the
jsxconfig in our code.Wrong version of react
Our sandboxes are created with React 19; our sample code is React 18. Our sample code uses
defaultProps, which is no longer present in React 19.Solution: add ts-ignores to defaultProps usage.
skipLibChecktofalse.@types/mdxand@mdx-js/reactdo not support React 19No idea how this is working on my repos; I'm importing from
@storybook/addon-docs/blocksjust the same, but the two packages are still using the now removed globalJSXnamespace instead ofReact.JSX.@storybook/reacthas type errors in its production filesThis is what #32442 is there to fix.
Still to do:
Work out how to make this run on an opt-in basis for our typescript sandboxes.
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 canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
Updated On: 2025-09-12 18:27:24 UTC
This PR introduces TypeScript type checking capabilities for Storybook sandboxes by adding a new
check-sandboxtask. The implementation addresses several compatibility issues between the development environment (React 18) and sandbox environments (React 19), as well as differences in TypeScript configurations between Storybook's existing setup and modern tooling like Vite.The core changes include:
New Infrastructure: A new
check-sandboxtask that runsyarn typecheckon individual sandboxes, with proper dependency management and existence checks. The task integrates into the existing task system and follows established patterns.TypeScript Configuration Management: The
prepareViteSandboxfunction modifies generated sandboxes to use less strict TypeScript settings, specifically disablingerasableSyntaxOnly(to allow enum usage),noUnusedLocals(to handle unnecessary React imports), andskipLibCheck(to enable checking of Storybook's own types).React Version Compatibility: Multiple template files have been updated with
@ts-ignorecomments to handledefaultPropsusage, which was removed in React 19 but is still valid in React 18 development environments. The comments are well-documented to explain this version mismatch.Dependency Management: Removed the TypeScript version pin that was forcing sandboxes to use an older version than expected by modern templates. Added missing type definitions like
@types/lodash-esand@types/prop-typesto support proper type checking.Template Cleanup: Various template files have been cleaned up to remove unused parameters and fix import patterns (like correcting CSS imports from named to default imports) to satisfy stricter TypeScript checking.
This change integrates with Storybook's existing sandbox infrastructure and follows the established task dependency model, where the
check-sandboxtask depends on thesandboxtask being completed first. The implementation is designed to be opt-in and focused initially on React Vite templates.Confidence score: 3/5
Summary by CodeRabbit