82-upgrade-typescript #83
Conversation
🦋 Changeset detectedLatest commit: cbfced8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded@agneym has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
WalkthroughSwitches the repo to the experimental TSGo flow and tsdown build, adds TS native-preview tooling and type-check scripts, tightens playground TypeScript config, migrates many runtime React imports to type-only imports, and applies several type-cast/typing adjustments and small test cleanups. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
playground/src/Playground.tsx (1)
63-63: Explicit type annotations are good but should be unnecessary.The explicit
width: numbertype annotations are redundant ifDraggable's prop types correctly specify the callback signatures. Once the type safety issue in lines 17-18 is resolved, these annotations should become unnecessary.Also applies to: 71-71
playground/src/TabStyles.tsx (1)
6-7: Update library versions and explore TypeScript compatibility improvements for styled tab components.All five styled components (StyledTabs, StyledTabList, StyledTab, StyledTabPanels, StyledTabPanel) cast to
any, disabling type checking for props validation, theme property access, and component API usage.Goober has been updated to 2.1.18 and @reach/tabs is available at 0.18.0, but upgrading alone may not resolve the type safety bypasses. Known compatibility issues exist when wrapping components with required props.
Consider:
- Testing updated versions to see if type compatibility improves
- Creating minimal type definitions that preserve props and theme typing
- Documenting this technical debt—especially relevant since @reach/tabs is an inactive project with no releases in the past 12 months
playground/src/Editor/index.tsx (1)
14-15: Type safety bypass in goober styled components wrapping @reach/tabs.The
as anycast disables type checking for all five tab component wrappers in TabStyles.tsx (Tabs, TabList, Tab, TabPanels, TabPanel) and extends to Editor/index.tsx. While this resolves the immediate type incompatibility, it completely bypasses type safety.Consider investigating:
- A more targeted type assertion like
as unknown as ComponentTypeto preserve some type safety- Whether a custom type wrapper or helper could resolve the incompatibility without full type erasure
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.changeset/solid-lands-crash.md(1 hunks).vscode/extensions.json(1 hunks).vscode/settings.json(1 hunks)package.json(1 hunks)playground/package.json(1 hunks)playground/scripts/test-utils.tsx(1 hunks)playground/src/Draggable/index.tsx(1 hunks)playground/src/Draggable/useDrag.ts(1 hunks)playground/src/Editor/EditorSetup.tsx(2 hunks)playground/src/Editor/index.tsx(4 hunks)playground/src/Playground.tsx(2 hunks)playground/src/Result/Console.tsx(1 hunks)playground/src/Result/ErrorDisplay.tsx(1 hunks)playground/src/Result/Frame.tsx(2 hunks)playground/src/Result/index.tsx(2 hunks)playground/src/TabStyles.tsx(3 hunks)playground/src/__tests__/Editor.test.tsx(0 hunks)playground/src/__tests__/Frame.test.tsx(0 hunks)playground/src/utils/ThemeProvider.tsx(1 hunks)playground/src/utils/constructSnippet.ts(1 hunks)playground/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (2)
- playground/src/tests/Editor.test.tsx
- playground/src/tests/Frame.test.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
playground/src/utils/constructSnippet.ts (1)
playground/src/types.ts (1)
ISnippet(1-5)
playground/src/utils/ThemeProvider.tsx (1)
playground/src/utils/theme.ts (2)
theme(63-102)ColorMode(104-104)
playground/scripts/test-utils.tsx (2)
playground/src/utils/ThemeProvider.tsx (2)
useTheme(9-9)ThemeProvider(17-27)playground/src/utils/theme.ts (1)
getTheme(110-113)
playground/src/Editor/index.tsx (2)
playground/src/TabStyles.tsx (1)
StyledTabs(7-16)playground/src/types.ts (2)
ITabConfig(10-14)IEditorTabs(7-7)
playground/src/Playground.tsx (2)
playground/src/utils/ThemeProvider.tsx (1)
useTheme(9-9)example/src/index.js (1)
snippet(8-24)
🪛 GitHub Actions: Node CI
.vscode/extensions.json
[error] 1-1: Formatting issues detected by oxfmt --check. Run without --check to fix.
playground/tsconfig.json
[error] 1-1: Formatting issues detected by oxfmt --check. Run without --check to fix.
🔇 Additional comments (29)
playground/src/Draggable/index.tsx (1)
1-2: LGTM! Type-only imports correctly applied.The separation of runtime imports from type-only imports follows TypeScript best practices and aligns with the
verbatimModuleSyntaxcompiler option enabled in this PR.playground/src/utils/constructSnippet.ts (1)
1-1: LGTM! Type-only import correctly applied.Since
ISnippetis only used in type positions, the type-only import is appropriate and improves build performance.playground/src/Draggable/useDrag.ts (1)
1-2: LGTM! Type-only import correctly applied.
RefObjectis only used in type annotations, making the type-only import appropriate and consistent with the PR's type-import strategy.playground/src/Editor/EditorSetup.tsx (3)
1-2: LGTM! Type-only import correctly applied.The separation of runtime (
Fragment) and type-only (FC) imports is correct and follows the type-import strategy.
8-8: LGTM! Type-only import correctly applied.
IEditorTabsis only used in type positions, making the type-only import appropriate.
34-34: LGTM! Cleaner destructuring.Removing unused
classNameandstylefrom the render prop destructuring simplifies the code without affecting functionality.playground/src/Playground.tsx (1)
1-2: LGTM! Type-only imports correctly applied.The type-only imports for
FC,DefaultTheme,ISnippet,IEditorTabs,IResultTabs, andColorModefollow TypeScript best practices and align with theverbatimModuleSyntaxoption.Also applies to: 4-5, 9-9, 11-11
playground/src/utils/ThemeProvider.tsx (3)
1-2: LGTM! Type-only imports correctly applied.The separation of runtime imports from type-only imports follows best practices and aligns with the stricter TypeScript configuration.
Also applies to: 4-4
11-17: LGTM! Explicit props interface improves type safety.The new
ThemeProviderPropsinterface provides clear, explicit typing for the component's props, which is especially beneficial with the stricter compiler options enabled in this PR.
25-25: LGTM! Nullish coalescing is the correct choice.Using
??instead of||ensures that onlynullorundefinedvalues fall back toconsolidatedTheme, preserving any falsy values inuserTheme(though unlikely in this context, it's still more semantically correct).playground/tsconfig.json (1)
1-29: File is already correctly formatted; no oxfmt action required.The tsconfig.json configuration is already properly formatted. The strict compiler options (
noUncheckedIndexedAccess,exactOptionalPropertyTypes, etc.) don't introduce compilation errors—the codebase shows no violations of these constraints in the TypeScript source files.playground/scripts/test-utils.tsx (3)
1-4: LGTM! Type-only imports follow best practices.The migration to type-only imports for React types (FC, ReactElement, ReactNode) and RenderOptions is appropriate and aligns with the TSGo upgrade. The namespace import pattern for @testing-library/react allows clear distinction between the custom render and the library's render function.
12-12: LGTM! Explicit typing improves type safety.Adding explicit FC typing for AllProviders with the children prop type is a good improvement that enhances type safety without changing runtime behavior.
20-22: LGTM! Function declaration maintains clarity.The refactor from an arrow function to a function declaration is appropriate and maintains the same functionality while working well with the namespace import pattern.
.changeset/solid-lands-crash.md (1)
1-5: LGTM! Changeset properly documents the upgrade.The changeset correctly identifies this as a minor update and clearly describes the TypeScript to tsgo upgrade with type error fixes.
playground/src/Result/ErrorDisplay.tsx (1)
1-1: LGTM! Type-only import aligns with TSGo migration.The migration to type-only import for FC is consistent with the broader TypeScript upgrade strategy and follows best practices.
.vscode/settings.json (1)
2-2: LGTM! VSCode configuration enables TSGo support.Enabling the experimental TSGo feature in VSCode aligns with the PR's objective to upgrade to TSGo and provides proper IDE support for the new TypeScript tooling.
playground/package.json (1)
45-46: LGTM! Type checking script properly configured.The new type:check script using
tsgo --noEmitis appropriate for the TSGo migration. The --noEmit flag correctly ensures only type checking without JavaScript emission. The trailing comma on line 45 follows good JSON formatting practices.playground/src/Result/Frame.tsx (3)
1-2: LGTM! Type-only import pattern is correct.Splitting the imports to use type-only import for FC while importing hooks as values is the correct pattern for the TSGo migration and follows TypeScript best practices.
5-5: LGTM! Type-only import for ISnippet is appropriate.Using type-only import for the ISnippet interface follows the consistent pattern established across the codebase.
31-31: LGTM! Improved error handling prevents runtime issues.The
instanceof Errorcheck before accessing.messageis a defensive improvement that prevents runtime errors when the catch block receives non-Error values. TheString(err)fallback ensures proper error display.package.json (2)
18-19: LGTM! Type checking workflow properly integrated.The new type:check script correctly delegates to the playground package's type checking, maintaining a clean separation of concerns in the monorepo structure.
24-24: Note the development preview nature and known limitations.@typescript/native-preview is a preview CLI of the native TypeScript compiler port. The version 7.0.0-dev.20251219.1 is the latest version, but be aware that not all features are implemented yet, and this package is intended for testing and experimentation.
Key limitations:
- Missing command-line functionality like --build and --declaration emit, and editor features like auto-imports, find-all-references, and rename
- Some JavaScript codebases may see more errors than before with type-checking changes
- Potential conflicts if dependencies use earlier TypeScript versions
Verify this preview build is acceptable for your team's needs given these constraints.
playground/src/Result/index.tsx (2)
1-2: LGTM! Import pattern consistent with TSGo migration.The split import pattern (hooks as values, FC as type) is consistent with the broader TypeScript upgrade and follows best practices.
11-11: LGTM! Type-only imports for interfaces.Using type-only imports for ISnippet, ITabConfig, and IResultTabs is the correct approach and maintains consistency across the codebase.
playground/src/Result/Console.tsx (2)
1-1: LGTM: Type-only import aligns with TS best practices.The type-only import for
FCis appropriate since it's only used as a type annotation.
3-4: No types package available for @agney/react-inspector.The package lacks built-in TypeScript support and no community-maintained @types package exists, making the
@ts-expect-errordirective appropriate for now. Consider eventually migrating to a modern alternative with first-class TypeScript support if type safety becomes important for this component.playground/src/Editor/index.tsx (2)
1-4: LGTM: Type-only imports improve type safety.Separating runtime imports (
useMemo) from type-only imports (FC,IEditorTabs,ISnippet,ITabConfig) aligns with TypeScript best practices and can improve tree-shaking.
27-39: Dependency array is correct and complete.The dependency array
[code.markup, code.css, code.javascript]correctly captures all properties of thecodeparameter that are accessed within the useMemo callback. The ISnippet interface contains exactly these three properties, and all are referenced in the callback. No other dependencies are missing.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const StyledDraggable = styled(Draggable as any)` |
There was a problem hiding this comment.
Type safety compromised by casting Draggable to any.
Casting Draggable to any bypasses TypeScript's type checking and could mask legitimate type errors. While this may be a quick workaround for goober's styled() type incompatibility, it eliminates type safety for the styled component.
Consider these alternatives:
- Define an explicit type assertion that preserves prop types
- Use goober's typing utilities if available
- Create a wrapper component instead of casting
🔎 Alternative approach using a typed wrapper
-// eslint-disable-next-line @typescript-eslint/no-explicit-any
-const StyledDraggable = styled(Draggable as any)`
+const DraggableWrapper = styled("div")`
border: 0.1em solid ${(props) => props.theme.container.borderColor};
display: flex;
min-height: ${(props) => props.theme.container.minHeight};
${media.phone} {
flex-direction: column;
}
`;
+
+const StyledDraggable: FC<React.ComponentProps<typeof Draggable>> = (props) => (
+ <DraggableWrapper>
+ <Draggable {...props} />
+ </DraggableWrapper>
+);📝 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.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| const StyledDraggable = styled(Draggable as any)` | |
| const DraggableWrapper = styled("div")` | |
| border: 0.1em solid ${(props) => props.theme.container.borderColor}; | |
| display: flex; | |
| min-height: ${(props) => props.theme.container.minHeight}; | |
| ${media.phone} { | |
| flex-direction: column; | |
| } | |
| `; | |
| const StyledDraggable: FC<React.ComponentProps<typeof Draggable>> = (props) => ( | |
| <DraggableWrapper> | |
| <Draggable {...props} /> | |
| </DraggableWrapper> | |
| ); |
🤖 Prompt for AI Agents
In playground/src/Playground.tsx around lines 17-18, the code casts Draggable to
any which removes type safety; replace the any cast by creating a typed wrapper
component that preserves Draggable's props (use React.ComponentProps<typeof
Draggable> or a DraggableProps interface) and forwardRef the ref/props through
to the original Draggable, then pass that wrapper to goober's styled(...) so
styled component keeps proper typings; alternatively, if goober exposes typings,
use styled<ComponentProps<...>>(Draggable) with the correct generic instead of
casting to any.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
playground/tsconfig.json (1)
1-29: Consider addingincludeandexcludefor explicit file control.The configuration lacks
includeandexcludefields, which means TypeScript will process all.ts/.tsxfiles in the directory tree (exceptnode_modules). For better clarity and potential performance, consider explicitly defining which files to process.📋 Example configuration
{ "compilerOptions": { ... - } + }, + "include": ["**/*.ts", "**/*.tsx"], + "exclude": ["node_modules", "dist", "build"] }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.vscode/extensions.json(1 hunks)playground/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .vscode/extensions.json
🔇 Additional comments (4)
playground/tsconfig.json (4)
5-7: LGTM! Target and library configuration is appropriate.The
"esnext"target with DOM libraries and Jest types is well-suited for a modern playground project with testing.
13-18: Excellent strictness improvements!These additional type-safety flags will catch many potential bugs:
noUncheckedIndexedAccessprevents undefined access errorsexactOptionalPropertyTypesenforces stricter optional handling- The unused variable/parameter checks help eliminate dead code
This is a strong type-safety posture for the project.
3-4: "module": "preserve" and "moduleResolution": "bundler" are a standard, documented configuration.These options are recommended for code that will be bundled. The "bundler" setting is designed for use with bundlers and supports package.json "imports" and "exports", but unlike Node.js resolution modes, bundler never requires file extensions on relative paths in imports. The "preserve" module setting is the best choice whenever a runtime or bundler is operating on raw .ts files, since it implies no transformation. Both options require TypeScript 5.0 or later, which is standard for modern projects.
9-11: Remove unnecessary declaration output options unless this is published as a library or uses project references.Declaration files generate .d.ts type definition files that describe a module's external API, intended for library packages. For a playground project,
declarationanddeclarationMapare unnecessary overhead unless you're publishing type definitions or using project references in a monorepo. Verify whether these are needed before keeping them enabled.
| "jsx": "react-jsx", | ||
| "verbatimModuleSyntax": true, | ||
| "isolatedModules": true, | ||
| "noUncheckedSideEffectImports": true, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for TypeScript version
find . -maxdepth 2 -name "package.json" -type f | while read f; do
echo "=== $f ==="
cat "$f" | jq '.devDependencies.typescript // .dependencies.typescript // "not found"' 2>/dev/null || echo "Failed to parse"
done
# Check for package-lock.json or yarn.lock
if [ -f "package-lock.json" ]; then
echo "=== package-lock.json ==="
cat package-lock.json | jq '.packages."node_modules/typescript".version // "not found"' 2>/dev/null
elif [ -f "yarn.lock" ]; then
echo "=== yarn.lock ==="
grep -A 1 "typescript@" yarn.lock | head -n 5
fiRepository: agneym/playground
Length of output: 181
Remove or upgrade TypeScript before using noUncheckedSideEffectImports.
The noUncheckedSideEffectImports flag requires TypeScript 5.6 or later. The project currently specifies "^4.4.4" in package.json, which will cause an "Unknown compiler option" error. Either upgrade TypeScript to 5.6+ or remove this option from tsconfig.json.
🤖 Prompt for AI Agents
In playground/tsconfig.json around line 25 the compiler option
"noUncheckedSideEffectImports" is set but this option requires TypeScript 5.6+,
while package.json currently pins TypeScript to ^4.4.4; fix by either (A)
upgrading the project's TypeScript dependency in package.json to a 5.6+ release
(e.g., update devDependency to ^5.6.0, run npm/yarn install, and verify builds)
or (B) remove the "noUncheckedSideEffectImports" entry from tsconfig.json so the
config is compatible with the existing TypeScript version.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
playground/package.json (1)
58-58: Packages are using secure versions; consider exact pinning for React Compiler.@rollup/plugin-babel 6.1.0 is current and poses no known security risks. babel-plugin-react-compiler 1.0.0 is the latest stable release, and tsdown 0.18.1 was released 2 days ago with no known vulnerabilities.
However, React's documentation recommends pinning babel-plugin-react-compiler to an exact version (e.g., 1.0.0) rather than a SemVer range (e.g., ^1.0.0) if test coverage is not comprehensive. Consider using exact pinning for this package to reduce unexpected behavior from minor/patch updates.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
playground/.babelrc(0 hunks)playground/package.json(3 hunks)playground/tsdown.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- playground/.babelrc
🧰 Additional context used
🪛 GitHub Actions: Node CI
playground/tsdown.config.ts
[error] 1-1: Format issues found by oxfmt --check. Run 'oxfmt' (without --check) to fix formatting in playground/tsdown.config.ts. Command 'pnpm format:check' failed with exit code 1.
🔇 Additional comments (2)
playground/package.json (1)
42-43: LGTM: Build and type-check scripts updated correctly.The migration from
microbundletotsdownfor build scripts and the addition of thetype:checkscript usingtsgoalign well with the PR objectives.Also applies to: 46-46
playground/tsdown.config.ts (1)
11-16: The configuration shown is valid. parserOpts is a standard Babel configuration option since Babel 6.16.0, and the structure withparserOptsandpluginsas sibling root-level properties aligns with how Babel plugins receive configuration. Since Rolldown supports @rollup/plugin-babel out of the box via Oxc, and tsdown uses Rolldown with a Rollup-compatible API, this configuration pattern is appropriate. No changes are needed.
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.