chore: migrate from TSDX to Vite, Vitest, and Biome - #4
Conversation
- Replace TSDX with Vite for faster builds and modern tooling - Use Vitest for testing with improved performance - Adopt Biome for unified linting and formatting - Update package.json to use new build scripts and dependencies - Configure TypeScript for Vite with bundler mode - Fix type safety issues by replacing 'any' with proper types - Update exports in package.json for better module resolution
|
Warning Rate limit exceeded@hideokamoto has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 39 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 selected for processing (6)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe changes represent a comprehensive tooling and build system modernization, migrating from tsdx to vite for building, replacing prettier/husky with biome for linting and formatting, adopting vitest for testing, and updating TypeScript configuration to modern standards with improved type safety in source code. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
Summary of ChangesHello @hideokamoto, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant modernization effort by migrating the project's core tooling from TSDX to a more contemporary stack comprising Vite for builds, Vitest for testing, and Biome for unified linting and formatting. This transition aims to boost development efficiency, improve build and test performance, and establish a consistent code style, ultimately leading to a more robust and maintainable codebase. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the project from TSDX to a more modern stack with Vite, Vitest, and Biome. The configuration files for the new tools are well-structured, and the changes in package.json reflect the migration. I've also noticed the improvements in type safety by removing any casts.
My review includes a few important points:
- Critical: Several dependencies in
package.jsonare pinned to versions that don't exist, which will break the build. - High: The Biome configuration points to a non-existent schema URL.
- Medium: I've suggested a couple of minor refactorings in
src/index.tsxandvite.config.tsfor simplification and clarity.
Overall, this is a great step forward for the project's tooling. Addressing the dependency versions is the most critical part.
| "devDependencies": { | ||
| "@size-limit/preset-small-lib": "^5.0.3", | ||
| "@biomejs/biome": "^2.3.5", | ||
| "@types/react": "^19.0.0", | ||
| "@types/react-dom": "^19.0.0", | ||
| "husky": "^7.0.2", | ||
| "@vitejs/plugin-react": "^5.1.1", | ||
| "@vitest/ui": "^4.0.9", | ||
| "jsdom": "^27.2.0", | ||
| "next": "^16.0.0", | ||
| "np": "^10.0.0", | ||
| "react": "^19.0.0", | ||
| "react-dom": "^19.0.0", | ||
| "size-limit": "^5.0.3", | ||
| "tsdx": "^0.14.1", | ||
| "tslib": "^2.3.1", | ||
| "typescript": "^5.7.0" | ||
| "typescript": "^5.7.0", | ||
| "vite": "^7.2.2", | ||
| "vite-plugin-dts": "^4.5.4", | ||
| "vitest": "^4.0.9" | ||
| } |
There was a problem hiding this comment.
Several dependencies are listed with versions that do not exist on the public npm registry (e.g., @biomejs/biome@^2.3.5, vite@^7.2.2, vitest@^4.0.9, next@^16.0.0). This will cause the dependency installation to fail. Please correct these versions to existing, published versions.
| @@ -0,0 +1,41 @@ | |||
| { | |||
| "$schema": "https://biomejs.dev/schemas/2.3.5/schema.json", | |||
There was a problem hiding this comment.
The schema version 2.3.5 does not appear to be a valid published version for Biome, which can cause issues with schema validation in your IDE. This seems to correspond to the version in package.json, which is also not a valid version for @biomejs/biome. Please use a valid version for both the package and the schema URL. For example, the latest stable version is 1.8.3.
| const adsbygoogle = window.adsbygoogle || []; | ||
| window.adsbygoogle = adsbygoogle; | ||
| adsbygoogle.push({}); |
There was a problem hiding this comment.
This logic is correct, but it can be simplified. Now that window.adsbygoogle is properly typed, you can use a more concise one-liner which is both readable and type-safe.
| const adsbygoogle = window.adsbygoogle || []; | |
| window.adsbygoogle = adsbygoogle; | |
| adsbygoogle.push({}); | |
| (window.adsbygoogle = window.adsbygoogle || []).push({}); |
| build: { | ||
| lib: { | ||
| entry: resolve(__dirname, 'src/index.tsx'), | ||
| name: 'NextGoogleAds', | ||
| formats: ['es', 'cjs'], | ||
| fileName: format => `index.${format === 'es' ? 'mjs' : 'cjs'}`, | ||
| }, | ||
| rollupOptions: { | ||
| external: ['react', 'react-dom', 'next/script'], | ||
| output: { | ||
| globals: { | ||
| react: 'React', | ||
| 'react-dom': 'ReactDOM', | ||
| }, | ||
| }, | ||
| }, | ||
| sourcemap: true, | ||
| }, |
There was a problem hiding this comment.
The lib.name and rollupOptions.output.globals options are only used for UMD and IIFE formats. Since you are only building ES and CJS modules, these options have no effect and can be removed to simplify the configuration.
build: {
lib: {
entry: resolve(__dirname, 'src/index.tsx'),
formats: ['es', 'cjs'],
fileName: format => `index.${format === 'es' ? 'mjs' : 'cjs'}`,
},
rollupOptions: {
external: ['react', 'react-dom', 'next/script'],
},
sourcemap: true,
},![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (1)
18-19: Critical: Update Node.js engine constraint to match dependency requirements.The pipeline warning indicates that dependencies require Node.js >=20 or >=22, but the engines field specifies >=10. This mismatch will cause installation and CI failures.
Apply this diff to update the Node.js requirement:
"engines": { - "node": ">=10" + "node": ">=20" },Additionally, verify your CI workflows use Node.js 20 or later.
src/index.tsx (1)
1-83: Address the mixed exports warning from the build pipeline.The pipeline warning indicates that mixing named and default exports may cause consumer confusion, requiring
NextGoogleAds.defaultto access the default export in some environments.Consider adding this to
vite.config.tsto explicitly configure export behavior:// In vite.config.ts build.lib section build: { lib: { // ... existing config }, rollupOptions: { // ... existing config output: { exports: 'named', // or 'auto' to suppress the warning // ... existing globals }, }, }Alternatively, if the default export is essential, document this requirement for CommonJS consumers.
🧹 Nitpick comments (1)
vitest.config.ts (1)
1-11: LGTM! Consider removing empty setupFiles array.The Vitest configuration is correct and properly integrates React support with jsdom environment.
If no setup files are needed, consider removing the empty array for cleaner configuration:
test: { globals: true, environment: 'jsdom', - setupFiles: [], },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
biome.json(1 hunks)package.json(3 hunks)src/index.tsx(3 hunks)test/blah.test.tsx(0 hunks)tsconfig.json(1 hunks)vite.config.ts(1 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- test/blah.test.tsx
🧰 Additional context used
🪛 GitHub Actions: CI
vite.config.ts
[error] 37-37: ESModule error: require() of ES Module /home/runner/work/next-google-ads/next-google-ads/node_modules/@vitejs/plugin-react/dist/index.js from vite.config.ts is not supported. Use a dynamic import() instead.
package.json
[warning] 1-1: EBADENGINE Unsupported engine: some dependencies require Node.js >=20 or >=22. Current: Node 18.x. Consider upgrading Node.js to satisfy engine constraints.
src/index.tsx
[warning] 1-1: Entry module "src/index.tsx" is using named and default exports together. Consumers of your bundle will have to use NextGoogleAds.default to access the default export, which may not be what you want. Use output.exports: "named" to disable this warning.
🔇 Additional comments (7)
biome.json (1)
1-41: LGTM! Well-configured Biome setup.The Biome configuration is well-structured with sensible defaults for formatting, linting, and import organization. The VCS integration and file inclusion patterns align well with the project structure.
tsconfig.json (1)
4-30: LGTM! Modern TypeScript configuration for Vite.The updated configuration appropriately targets ES2020 with bundler module resolution and includes all necessary options for a Vite-based library build. The
noEmit: trueis correct here since Vite handles the actual build andvite-plugin-dtsgenerates declarations.package.json (2)
4-13: LGTM! Proper dual package configuration.The
exports,module,types, andmainfields are correctly configured for dual ESM/CJS support with proper TypeScript declarations.
22-31: LGTM! Well-structured build and test scripts.The migration from TSDX to Vite/Vitest/Biome scripts is complete and well-organized, providing good developer experience with watch modes and UI options.
src/index.tsx (2)
2-8: LGTM! Excellent type safety improvement.The migration to type-only React imports and the introduction of
WindowWithAdsbygoogleinterface significantly improve type safety compared to the previous approach.
63-65: LGTM! Much safer than casting toany.The typed approach to accessing
window.adsbygoogleis a significant improvement over the previous(window as any)pattern.vite.config.ts (1)
6-31: LGTM! Well-configured Vite library build.Once the ESModule import issue is resolved, this configuration properly sets up:
- Dual ESM/CJS output formats
- TypeScript declaration generation
- Correct externalization of peer dependencies
- Source maps for debugging
- Add "type": "module" to package.json for ESM support - Update Node.js requirement to >=20.19.0 for Vite compatibility - Use fileURLToPath for __dirname in ESM context - Remove UMD/IIFE-specific options from Vite config - Simplify adsbygoogle initialization code - Disable noAssignInExpressions rule for cleaner code
- Remove Node.js 18.x from test matrix (requires >=20.19.0) - Remove --ci flag from vitest command (automatically detected) - Remove size-limit workflow (TSDX dependency removed)
- Remove cross-platform matrix (Windows, macOS) - Use only Ubuntu with Node 20.x - YAGNI: library has no platform-specific code
- Add Node.js version matrix (20.x LTS, 22.x latest) - Keep OS to Linux only for simplicity
2 participants
Summary by CodeRabbit