Fix commonjs/esm/types output #610
Conversation
|
Warning Rate limit exceeded@vio has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 25 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 (6)
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
70-84: Add Vite as a root devDependency so Vite builds in tests resolve.
Tests import build from 'vite' directly; without a root-level Vite install, resolution may fail when running from the subfolder.Proposed devDependencies excerpt:
{ "devDependencies": { "@release-it/conventional-changelog": "10.0.1", "@rollup/plugin-typescript": "12.1.4", "@tsconfig/node18": "18.2.4", "@types/node": "24.3.0", "dotenv": "17.2.1", "husky": "9.1.7", "memfs": "4.36.3", "release-it": "19.0.4", "rimraf": "6.0.1", "rollup": "4.46.4", "tslib": "2.8.1", "typescript": "5.9.2", "vitest": "3.2.4", "vite": "^7.0.0" } }
🧹 Nitpick comments (8)
test/package/rollup/.gitignore (1)
1-2: Broaden ignore patterns for test artifacts (keep suites clean).Add common lockfiles, caches, and deps dirs.
package-lock.json dist* +node_modules/ +yarn.lock +pnpm-lock.yaml +.rollup.cache/ +.vite/test/package/rollup/options.test.ts (2)
21-28: Avoid brittle byte-size assertions.Size 73 is version/flag-sensitive; assert type and positivity instead.
- expect(stats).toMatchObject({ - assets: [ - { - name: 'index.js', - size: 73, - }, - ], - }) + expect(stats).toMatchObject({ + assets: [ + { + name: 'index.js', + size: expect.any(Number), + }, + ], + }); + expect(stats.assets[0].size).toBeGreaterThan(0);
38-45: Same here: relax the size check for the builder function case.- expect(stats).toMatchObject({ - assets: [ - { - name: 'index.js', - size: 73, - }, - ], - }) + expect(stats).toMatchObject({ + assets: [ + { + name: 'index.js', + size: expect.any(Number), + }, + ], + }); + expect(stats.assets[0].size).toBeGreaterThan(0);test/package/vite/.gitignore (1)
1-2: Mirror broader ignores here too.package-lock.json dist* +node_modules/ +yarn.lock +pnpm-lock.yaml +.vite/ +.rollup.cache/test/package/vite/src/index.js (1)
1-4: Minor: silence console lint in case linting is enabled.+/* eslint-disable no-console */ (() => { const message = 'hello world!'; console.log(message); })();test/package/vite/package.json (1)
2-4: Mark test package as private to avoid accidental publish.Apply this diff:
{ "name": "rollup-plugin-webpack-stats-test-vite", "version": "1.0.0", + "private": true, "description": "",test/package/vite/options.test.ts (1)
5-12: Remove unused memfs reset.
vol is imported but not used by Vite builds here.Apply this diff:
-import { vol } from 'memfs'; - ... describe('vite options', () => { - beforeEach(() => { - vol.reset(); - }); + // no-oppackage.json (1)
53-55: Build before running package tests to ensure dist exists.
Pre-building avoids tests consuming stale/missing artifacts.Apply this diff:
- "test:package": "npm run test:package:rollup && npm run test:package:vite", + "test:package": "npm run build && npm run test:package:rollup && npm run test:package:vite",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
test/package/rollup/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.json(1 hunks)test/package/rollup/.gitignore(1 hunks)test/package/rollup/options.test.ts(1 hunks)test/package/rollup/package.json(0 hunks)test/package/vite/.gitignore(1 hunks)test/package/vite/case-options.mjs(1 hunks)test/package/vite/index.html(1 hunks)test/package/vite/options.test.ts(1 hunks)test/package/vite/package.json(1 hunks)test/package/vite/src/index.js(1 hunks)
💤 Files with no reviewable changes (1)
- test/package/rollup/package.json
🧰 Additional context used
🧬 Code graph analysis (3)
test/package/vite/case-options.mjs (1)
src/plugin.ts (1)
webpackStats(27-60)
test/package/vite/src/index.js (1)
test/package/src/index.js (1)
message(1-4)
package.json (1)
test/package/src/index.js (1)
message(1-4)
🔇 Additional comments (6)
test/package/rollup/options.test.ts (4)
7-7: Confirm default export shape from case-options.mjs.Ensure it default-exports an array with two configs; update to named import if not.
9-9: Suite rename reads clearer.Naming the suite “rollup options” improves test intent.
14-18: Do both configs exercise ESM and CJS outputs?Given the PR goal (“Fix commonjs/esm/types output”), ensure config[0] and config[1] cover different formats to validate the fix end-to-end.
Also applies to: 31-35
10-12: Validate memfs reset is effective.If the plugin writes via Node fs,
vol.reset()is a no-op; if it patchesfsto memfs, it’s fine. Confirm the intended wiring.test/package/vite/index.html (1)
1-10: LGTM: minimal HTML entry for Vite build.test/package/vite/package.json (1)
11-14: Confirm how Vite is resolved during tests.
If installs are not performed in this subfolder, importing 'vite' will rely on the root node_modules. Ensure Vite is a root devDependency (see root package.json comment). Otherwise, this devDependency here won’t be installed.
| assets: [ | ||
| { | ||
| name: 'assets/index.js', | ||
| size: 739, | ||
| }, | ||
| ], | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the size assertion resilient.
Exact byte size is brittle across Vite/Rollup versions and platforms. Assert presence and numeric size instead.
Apply this diff:
- expect(stats).toMatchObject({
- assets: [
- {
- name: 'assets/index.js',
- size: 739,
- },
- ],
- })
+ expect(stats).toMatchObject({
+ assets: [
+ {
+ name: 'assets/index.js',
+ size: expect.any(Number),
+ },
+ ],
+ });
+ // Optional: stronger check
+ // expect(stats.assets.find(a => a.name === 'assets/index.js')!.size).toBeGreaterThan(0);🤖 Prompt for AI Agents
In test/package/vite/options.test.ts around lines 21 to 27, the test currently
asserts an exact byte size for assets which is brittle; change the assertion to
verify the asset exists and that its size is a numeric value (and optionally >
0). Replace the exact size equality check with an assertion that the asset
object has a "size" property and that typeof size === "number" (or
expect(size).toBeGreaterThan(0)) so the test is resilient across Vite/Rollup
versions and platforms.
| expect(stats).toMatchObject({ | ||
| assets: [ | ||
| { | ||
| name: 'assets/index.js', | ||
| size: 739, | ||
| }, | ||
| ], | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same here: avoid hardcoding byte size.
Apply this diff:
- expect(stats).toMatchObject({
- assets: [
- {
- name: 'assets/index.js',
- size: 739,
- },
- ],
- })
+ expect(stats).toMatchObject({
+ assets: [
+ {
+ name: 'assets/index.js',
+ size: expect.any(Number),
+ },
+ ],
+ });📝 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.
| expect(stats).toMatchObject({ | |
| assets: [ | |
| { | |
| name: 'assets/index.js', | |
| size: 739, | |
| }, | |
| ], | |
| }) | |
| expect(stats).toMatchObject({ | |
| assets: [ | |
| { | |
| name: 'assets/index.js', | |
| size: expect.any(Number), | |
| }, | |
| ], | |
| }); |
🤖 Prompt for AI Agents
In test/package/vite/options.test.ts around lines 36 to 43, the test hardcodes
the asset byte size (739); replace that with a runtime-calculated size to avoid
brittle tests: read the built asset file (resolve the path to assets/index.js),
compute its byte length with Buffer.byteLength(fs.readFileSync(filePath)), and
assert the stats.assets[0].size equals that computed value (or use
expect.any(Number) if you prefer a looser assertion). Ensure you import fs and
path as needed and reference the correct output path when computing the size.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
test/package/rollup/case-config.ts (1)
1-10: Optional: Use a named import here to also validate type exportsTS is a good place to assert named exports/types. Consider:
-import webpackStats from 'rollup-plugin-webpack-stats'; +import { webpackStats } from 'rollup-plugin-webpack-stats';test/package/rollup/case-config.js (1)
1-1: Nit: ESLint disable may be unnecessary in a .js CJS configIf your ESLint config doesn’t enforce @typescript-eslint/no-require-imports on JS, you can drop this directive. If it does, keeping it is fine.
test/package/vite/configs.test.ts (2)
15-16: Typo: “rollup” -> “vite” in test title.Avoid confusion when scanning test output.
- test(`should build successfully with rollup ${testCase.type} config`, async () => { + test(`should build successfully with vite ${testCase.type} config`, async () => {
7-11: Ensure subpackage deps are installed in CI.These tests invoke Vite from the vite subpackage. Make sure CI runs installs there (e.g., npm ci --prefix test/package/vite) before executing tests.
test/package/vite/package.json (2)
3-6: Mark test package as private.Prevents accidental publish.
"name": "rollup-plugin-webpack-stats-test-vite", "version": "1.0.0", + "private": true, "description": "",
14-18: cross-env is no longer needed after test fix.Trim devDependencies to reduce noise.
"devDependencies": { - "cross-env": "^10.0.0", "rollup-plugin-webpack-stats": "*", "vite": "^7.0.0" }test/package/rollup/configs.test.ts (2)
18-19: Optional: assert against combined output for resilience.Rollup/vite can route logs to different streams; combining reduces flakiness.
- expect(stderr).toMatch('Stats saved to dist'); - expect(stderr).toMatch('created dist in'); + const out = `${stdout}\n${stderr}`; + expect(out).toMatch('Stats saved to dist'); + expect(out).toMatch('created dist in');
7-11: Ensure subpackage deps are installed in CI.Install rollup-related devDeps in test/package/rollup before running tests (e.g., npm ci --prefix test/package/rollup).
test/package/rollup/package.json (2)
3-6: Mark test package as private.Prevents accidental publish.
"name": "rollup-plugin-webpack-stats-test", "version": "1.0.0", + "private": true, "description": "",
14-20: cross-env devDep can be dropped.Not used by scripts after tests set env programmatically.
"devDependencies": { "@rollup/plugin-typescript": "^12.1.4", - "cross-env": "^10.0.0", "rollup": "^4.9.1", "rollup-plugin-webpack-stats": "*", "typescript": "^5.9.2" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
test/package/rollup/case-config.js(1 hunks)test/package/rollup/case-config.mjs(1 hunks)test/package/rollup/case-config.ts(1 hunks)test/package/rollup/configs.test.ts(1 hunks)test/package/rollup/package.json(1 hunks)test/package/vite/case-config.js(1 hunks)test/package/vite/case-config.mjs(1 hunks)test/package/vite/case-config.ts(1 hunks)test/package/vite/case-options.mjs(1 hunks)test/package/vite/configs.test.ts(1 hunks)test/package/vite/options.test.ts(1 hunks)test/package/vite/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/package/vite/case-options.mjs
- test/package/vite/options.test.ts
🧰 Additional context used
🧬 Code graph analysis (4)
test/package/vite/case-config.mjs (1)
src/plugin.ts (1)
webpackStats(27-60)
test/package/rollup/case-config.mjs (1)
test/package/rollup/case-config.js (1)
stats(3-3)
test/package/rollup/case-config.ts (1)
test/package/rollup/case-config.js (1)
stats(3-3)
test/package/vite/case-config.ts (1)
src/plugin.ts (1)
webpackStats(27-60)
🪛 GitHub Actions: ci
test/package/vite/configs.test.ts
[error] 1-1: Command failed: cross-env NO_COLOR=true npm run build-commonjs. /bin/sh: 1: cross-env: not found
[error] 1-1: Command failed: cross-env NO_COLOR=true npm run build-esm. /bin/sh: 1: cross-env: not found
[error] 1-1: Command failed: cross-env NO_COLOR=true npm run build-ts. /bin/sh: 1: cross-env: not found
test/package/rollup/configs.test.ts
[error] 1-1: Command failed: cross-env NO_COLOR=true npm run build-commonjs. /bin/sh: 1: cross-env: not found
[error] 1-1: Command failed: cross-env NO_COLOR=true npm run build-esm. /bin/sh: 1: cross-env: not found
[error] 1-1: Command failed: cross-env NO_COLOR=true npm run build-ts. /bin/sh: 1: cross-env: not found
🔇 Additional comments (6)
test/package/rollup/case-config.mjs (1)
1-10: Fix Rollup test configs: use defineConfig from ‘rollup’, import this plugin, and set output.format
- In test/package/rollup/case-config.{js,ts,mjs}, replace
withimport { defineConfig } from 'vite' // or require('vite') import stats from 'rollup-plugin-stats'import { defineConfig } from 'rollup' import <yourPlugin> from '<this-repo’s-rollup-plugin-package>'- Under
output, addformat: 'es',test/package/vite/case-config.mjs (1)
1-15: LGTM for ESM usage in ViteConfig shape and default import exercise the ESM path as intended.
test/package/vite/case-config.js (1)
2-16: LGTM for CJS usage in ViteThis exercises the CJS require path; config shape looks correct.
test/package/vite/case-config.ts (1)
4-15: LGTM.Config looks correct for the test purpose; namespacing outputs under assets/ avoids collisions in assertions.
test/package/vite/package.json (1)
14-18: Verify intended source for rollup-plugin-webpack-stats.If these tests should cover the local package (not the published one), switch to a local reference (workspace:* or file:../../..). Otherwise keep "*".
Would you like me to prep a workspace/file: change?
test/package/rollup/package.json (1)
14-20: Verify local vs published plugin under test.If the intention is to test the current branch, prefer workspace:* or file:../../.. instead of "*".
Want me to push the appropriate change based on your package manager?
| /* eslint-disable @typescript-eslint/no-require-imports */ | ||
| const { defineConfig } = require('vite'); | ||
| const stats = require('rollup-plugin-stats'); | ||
|
|
||
| module.exports = defineConfig({ | ||
| input: 'src/index.js', | ||
| output: { | ||
| dir: 'dist', | ||
| }, | ||
| plugins: [stats()], | ||
| }); |
There was a problem hiding this comment.
Fix: Point to Rollup and the correct plugin; add output.format
Align CJS Rollup config with the package under test and ensure Rollup output is valid.
-/* eslint-disable @typescript-eslint/no-require-imports */
-const { defineConfig } = require('vite');
-const stats = require('rollup-plugin-stats');
+/* eslint-disable @typescript-eslint/no-require-imports */
+const { defineConfig } = require('rollup');
+const webpackStatsPlugin = require('rollup-plugin-webpack-stats');
module.exports = defineConfig({
input: 'src/index.js',
output: {
- dir: 'dist',
+ dir: 'dist',
+ format: 'es',
},
- plugins: [stats()],
+ plugins: [webpackStatsPlugin()],
});📝 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 @typescript-eslint/no-require-imports */ | |
| const { defineConfig } = require('vite'); | |
| const stats = require('rollup-plugin-stats'); | |
| module.exports = defineConfig({ | |
| input: 'src/index.js', | |
| output: { | |
| dir: 'dist', | |
| }, | |
| plugins: [stats()], | |
| }); | |
| /* eslint-disable @typescript-eslint/no-require-imports */ | |
| const { defineConfig } = require('rollup'); | |
| const webpackStatsPlugin = require('rollup-plugin-webpack-stats'); | |
| module.exports = defineConfig({ | |
| input: 'src/index.js', | |
| output: { | |
| dir: 'dist', | |
| format: 'es', | |
| }, | |
| plugins: [webpackStatsPlugin()], | |
| }); |
🤖 Prompt for AI Agents
In test/package/rollup/case-config.js lines 1-11, the config incorrectly imports
defineConfig from Vite and is missing an explicit Rollup output format; update
the top-level requires to pull defineConfig from 'rollup' (i.e.
require('rollup')), keep or validate the stats plugin is the Rollup plugin, and
add output.format: 'cjs' to the output object so the Rollup config matches the
CJS package under test.
test/package/rollup/case-config.ts
Outdated
| import { defineConfig } from 'vite'; | ||
| import stats from 'rollup-plugin-stats'; | ||
|
|
||
| export default defineConfig({ | ||
| input: 'src/index.js', | ||
| output: { | ||
| dir: 'dist', | ||
| }, | ||
| plugins: [stats({})], | ||
| }); |
There was a problem hiding this comment.
Fix: Rollup config should import from rollup, use this repo’s plugin, and specify format
Same issues as the .mjs variant: wrong defineConfig source, wrong plugin, and missing output.format.
-import { defineConfig } from 'vite';
-import stats from 'rollup-plugin-stats';
+import { defineConfig } from 'rollup';
+import webpackStats from 'rollup-plugin-webpack-stats';
export default defineConfig({
input: 'src/index.js',
output: {
- dir: 'dist',
+ dir: 'dist',
+ format: 'es',
},
- plugins: [stats({})],
+ plugins: [webpackStats({})],
});📝 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.
| import { defineConfig } from 'vite'; | |
| import stats from 'rollup-plugin-stats'; | |
| export default defineConfig({ | |
| input: 'src/index.js', | |
| output: { | |
| dir: 'dist', | |
| }, | |
| plugins: [stats({})], | |
| }); | |
| import { defineConfig } from 'rollup'; | |
| import webpackStats from 'rollup-plugin-webpack-stats'; | |
| export default defineConfig({ | |
| input: 'src/index.js', | |
| output: { | |
| dir: 'dist', | |
| format: 'es', | |
| }, | |
| plugins: [webpackStats({})], | |
| }); |
🤖 Prompt for AI Agents
In test/package/rollup/case-config.ts lines 1-10, the config wrongly imports
defineConfig from 'vite', uses the external rollup-plugin-stats instead of this
repo's rollup plugin, and omits output.format; update the file to import
defineConfig from 'rollup', import the stats plugin implementation from this
repository (the local rollup plugin path used by the project), and add an
explicit output.format (e.g. 'es' or the format your package expects) to the
output object so the rollup config uses the correct plugin source and emits the
correct module format.
| const { stderr } = await exec(`cross-env NO_COLOR=true ${testCase.run}`); | ||
|
|
||
| expect(stderr).toMatch('Stats saved to dist'); | ||
| expect(stderr).toMatch('created dist in'); | ||
| }); |
There was a problem hiding this comment.
Fix CI break: stop using cross-env and set cwd/env via exec options.
Same issue as vite tests; also ensure we execute within the rollup subpackage.
-import { describe, expect, test } from 'vitest';
+import { describe, expect, test } from 'vitest';
+import path from 'node:path';
+import { fileURLToPath } from 'node:url';
@@
- testCases.forEach((testCase) => {
- test(`should build successfully with rollup ${testCase.type} config`, async () => {
- const { stderr } = await exec(`cross-env NO_COLOR=true ${testCase.run}`);
+ const __dirname = path.dirname(fileURLToPath(import.meta.url));
+ testCases.forEach((testCase) => {
+ test(`should build successfully with rollup ${testCase.type} config`, async () => {
+ const { stdout, stderr } = await exec(testCase.run, {
+ cwd: __dirname,
+ env: { ...process.env, NO_COLOR: 'true' },
+ });📝 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.
| const { stderr } = await exec(`cross-env NO_COLOR=true ${testCase.run}`); | |
| expect(stderr).toMatch('Stats saved to dist'); | |
| expect(stderr).toMatch('created dist in'); | |
| }); | |
| import { describe, expect, test } from 'vitest'; | |
| import path from 'node:path'; | |
| import { fileURLToPath } from 'node:url'; | |
| describe('rollup configs', () => { | |
| const __dirname = path.dirname(fileURLToPath(import.meta.url)); | |
| testCases.forEach((testCase) => { | |
| test(`should build successfully with rollup ${testCase.type} config`, async () => { | |
| const { stdout, stderr } = await exec(testCase.run, { | |
| cwd: __dirname, | |
| env: { ...process.env, NO_COLOR: 'true' }, | |
| }); | |
| expect(stderr).toMatch('Stats saved to dist'); | |
| expect(stderr).toMatch('created dist in'); | |
| }); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In test/package/rollup/configs.test.ts around lines 16 to 20, the test currently
spawns the command with a cross-env prefix and relies on shell env propagation;
update the exec invocation to remove the leading "cross-env NO_COLOR=true " from
the command, and instead pass { cwd: path.join(__dirname, '..', 'rollup'), env:
{ ...process.env, NO_COLOR: 'true' } } (or equivalent) as exec options so the
test runs in the rollup subpackage with NO_COLOR set via env rather than using
cross-env.
| test(`should build successfully with rollup ${testCase.type} config`, async () => { | ||
| const { stdout } = await exec(`cross-env NO_COLOR=true ${testCase.run}`); | ||
|
|
||
| expect(stdout).toMatch('Stats saved to'); | ||
| expect(stdout).toMatch('built in'); | ||
| }); |
There was a problem hiding this comment.
Fix CI break: stop using cross-env and set cwd/env via exec options.
cross-env isn’t available in CI and cwd isn’t set to the subpackage, causing failures. Pass NO_COLOR via env and run within the vite package folder.
Apply:
@@
-import { describe, expect, test } from 'vitest';
+import { describe, expect, test } from 'vitest';
+import path from 'node:path';
+import { fileURLToPath } from 'node:url';
@@
- testCases.forEach((testCase) => {
- test(`should build successfully with rollup ${testCase.type} config`, async () => {
- const { stdout } = await exec(`cross-env NO_COLOR=true ${testCase.run}`);
+ const __dirname = path.dirname(fileURLToPath(import.meta.url));
+ testCases.forEach((testCase) => {
+ test(`should build successfully with vite ${testCase.type} config`, async () => {
+ const { stdout } = await exec(testCase.run, {
+ cwd: __dirname,
+ env: { ...process.env, NO_COLOR: 'true' },
+ });
@@
expect(stdout).toMatch('Stats saved to');
expect(stdout).toMatch('built in');
});
});📝 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.
| test(`should build successfully with rollup ${testCase.type} config`, async () => { | |
| const { stdout } = await exec(`cross-env NO_COLOR=true ${testCase.run}`); | |
| expect(stdout).toMatch('Stats saved to'); | |
| expect(stdout).toMatch('built in'); | |
| }); | |
| import { describe, expect, test } from 'vitest'; | |
| import path from 'node:path'; | |
| import { fileURLToPath } from 'node:url'; | |
| // ... other setup/imports ... | |
| const __dirname = path.dirname(fileURLToPath(import.meta.url)); | |
| describe('vite configs', () => { | |
| testCases.forEach((testCase) => { | |
| test(`should build successfully with vite ${testCase.type} config`, async () => { | |
| const { stdout } = await exec(testCase.run, { | |
| cwd: __dirname, | |
| env: { ...process.env, NO_COLOR: 'true' }, | |
| }); | |
| expect(stdout).toMatch('Stats saved to'); | |
| expect(stdout).toMatch('built in'); | |
| }); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In test/package/vite/configs.test.ts around lines 15 to 20, the test currently
uses a shell-prefixed command with cross-env and relies on the wrong working
directory; update the exec call to stop using cross-env and instead pass
NO_COLOR via the exec options env (e.g. env: { ...process.env, NO_COLOR: 'true'
}) and set cwd to the vite package folder (use path.resolve relative to
__dirname to point to the package), so the command runs in the correct directory
with the environment variable provided by exec rather than cross-env.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/ci.yml (2)
54-65: Fix OS matrix usage and ensure Windows compatibility for bash script
- The job matrix includes windows-latest, but runs-on is hardcoded to ubuntu-latest. Windows variants won’t actually run, and the job name becomes misleading.
- The registry setup script is a bash script; on Windows runners, default shell is PowerShell. Force bash.
Apply:
test-package-rollup: name: Test package on node ${{ matrix.node }} / ${{ matrix.os }} / rollup v${{ matrix.rollup }} needs: - build - runs-on: ubuntu-latest + runs-on: ${{ matrix.os }} @@ - name: Setup local npm registry - run: ./scripts/setup-registry.sh + run: ./scripts/setup-registry.sh + shell: bashAlso applies to: 58-58, 94-96
65-73: Ensure matrix exclude matches types (quote node versions)Matrix node values are strings; excludes use unquoted numbers. Type mismatch can prevent exclusions from applying.
exclude: - os: windows-latest - node: 20 + node: '20' - os: windows-latest - node: 18 + node: '18' - rollup: 3 - node: 20 + node: '20' - rollup: 3 - node: 18 + node: '18'
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
86-88: Drop redundant root-level vitest installYou install deps in test/package/rollup later. Installing vitest at repo root is unnecessary and slows CI.
- - name: Install vitest - run: npm install vitest
98-103: Run tests inside the subpackage for claritySince dependencies are installed in test/package/rollup, execute the tests from that directory to avoid coupling to root scripts.
- name: Install test package dependencies run: | npm install npm install rollup@${{ matrix.rollup }} --force npx rollup -v working-directory: test/package/rollup - name: Test package - run: npm run test:package:rollup + run: npm run test:package + working-directory: test/package/rollupAlso applies to: 104-106
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/ci.yml(2 hunks)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
107-112: Dependency graph update looks correcttests now depends on test-package-rollup instead of the old job. Good.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/ci.yml (2)
54-75: Matrix os is defined but not used; runs-on is hardcoded to ubuntuYou define an
osaxis, exclude Windows combos, and even print it in the job name, butruns-onis fixed toubuntu-latest, so Windows never runs. Use the matrix value.- runs-on: ubuntu-latest + runs-on: ${{ matrix.os }}
94-101: Bash script will fail on Windows unless you force the shellWhen you switch to
windows-latest,./scripts/setup-registry.shwill default to PowerShell and fail. Force bash for cross-platform.- name: Setup local npm registry - run: ./scripts/setup-registry.sh + run: ./scripts/setup-registry.sh + shell: bash
🧹 Nitpick comments (4)
.github/workflows/ci.yml (4)
86-88: Redundant root-level “Install vitest” stepYou install
vitestat repo root, then runnpm installin the subpackage which should already declare devDeps. Drop the root install to save time and avoid version skew.- - name: Install vitest - run: npm install vitest ... - - name: Install test package dependencies + - name: Install test package dependencies run: | - npm install + npm install npm install rollup@${{ matrix.rollup }} --force npx rollup -v working-directory: test/package/rollupAlso applies to: 97-103, 104-106
136-138: Drop redundant root-level vitest install (vite job)Same redundancy as rollup job.
- - name: Install vitest - run: npm install vitest ... - - name: Install test package dependencies + - name: Install test package dependencies run: | - npm install + npm install npm install vite@${{ matrix.vite }} --force npx vite -v working-directory: test/package/viteAlso applies to: 147-153, 154-156
148-151: Fix YAML trailing space (yamllint error at line 149)There’s a trailing space after
npm install.- npm install + npm install
97-103: Prefer npm ci for deterministic installs (if lockfiles exist)If
test/package/rollupandtest/package/vitecontain lockfiles, switch tonpm cifor reproducible installs and speed; otherwise ignore.- npm install + npm ciPlease confirm
package-lock.jsonexists in both subpackages before applying.Also applies to: 147-153
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/ci.yml(2 hunks)test/package/rollup/package.json(1 hunks)test/package/vite/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/package/rollup/package.json
- test/package/vite/package.json
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 149-149: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
157-163: Tests aggregation looks goodUpdating
tests.needsto depend on the split jobs is correct.
| test-package-vite: | ||
| name: "vite v${{ matrix.vite }} / node ${{ matrix.node }} / ${{ matrix.os }}" | ||
| needs: | ||
| - build | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| node: ["22", "20", "18"] | ||
| os: [ubuntu-latest, windows-latest] | ||
| vite: ["5", "6", "7"] | ||
| exclude: | ||
| - os: windows-latest | ||
| node: 20 | ||
| - os: windows-latest | ||
| node: 18 | ||
| - vite: 7 | ||
| node: 18 | ||
| steps: |
There was a problem hiding this comment.
Matrix os is defined but not used; runs-on is hardcoded to ubuntu (vite job)
Same issue as rollup job; Windows variants won’t execute.
- runs-on: ubuntu-latest
+ runs-on: ${{ matrix.os }}📝 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.
| test-package-vite: | |
| name: "vite v${{ matrix.vite }} / node ${{ matrix.node }} / ${{ matrix.os }}" | |
| needs: | |
| - build | |
| runs-on: ubuntu-latest | |
| strategy: | |
| fail-fast: false | |
| matrix: | |
| node: ["22", "20", "18"] | |
| os: [ubuntu-latest, windows-latest] | |
| vite: ["5", "6", "7"] | |
| exclude: | |
| - os: windows-latest | |
| node: 20 | |
| - os: windows-latest | |
| node: 18 | |
| - vite: 7 | |
| node: 18 | |
| steps: | |
| test-package-vite: | |
| name: "vite v${{ matrix.vite }} / node ${{ matrix.node }} / ${{ matrix.os }}" | |
| needs: | |
| - build | |
| runs-on: ${{ matrix.os }} | |
| strategy: | |
| fail-fast: false | |
| matrix: | |
| node: ["22", "20", "18"] | |
| os: [ubuntu-latest, windows-latest] | |
| vite: ["5", "6", "7"] | |
| exclude: | |
| - os: windows-latest | |
| node: 20 | |
| - os: windows-latest | |
| node: 18 | |
| - vite: 7 | |
| node: 18 | |
| steps: |
🤖 Prompt for AI Agents
.github/workflows/ci.yml around lines 107 to 125: the test-package-vite job
defines a matrix.os but the job hardcodes runs-on: ubuntu-latest so Windows
variants never run; change runs-on to use the matrix value (runs-on: ${{
matrix.os }}), ensure the matrix.os contains the desired runners (ubuntu-latest,
windows-latest), and keep or adjust the existing exclude entries so the intended
combinations are still skipped.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/ci.yml (2)
54-74: Use matrix OS for runner (Windows variants currently skipped).Change runs-on to pick up matrix.os so Windows jobs actually execute.
- runs-on: ubuntu-latest + runs-on: ${{ matrix.os }}
94-103: Ensure bash for registry script and multi-line install on Windows.These steps will fail under PowerShell on windows-latest; force bash.
- name: Setup local npm registry - run: ./scripts/setup-registry.sh + run: ./scripts/setup-registry.sh + shell: bash - name: Install test package dependencies run: | npm install npm install rollup@${{ matrix.rollup }} --force npx rollup -v + shell: bash working-directory: test/package/rollup
♻️ Duplicate comments (2)
.github/workflows/ci.yml (2)
108-126: Matrix os is defined but not used; switch runner to matrix.os (vite job).Same fix as previously noted.
- runs-on: ubuntu-latest + runs-on: ${{ matrix.os }}
145-153: Force bash for registry + install; remove trailing space flagged by YAMLlint.Prevents Windows failures and fixes trailing spaces at Line 150.
- name: Setup local npm registry - run: ./scripts/setup-registry.sh + run: ./scripts/setup-registry.sh + shell: bash - name: Install test package dependencies run: | - npm install + npm install npm install vite@${{ matrix.vite }} --force npx vite -v + shell: bash working-directory: test/package/vite
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
86-88: Drop redundant root-level vitest install (saves time, unused).Tests use the subpackage’s devDeps; this step isn’t needed.
- - name: Install vitest - run: npm install vitest
54-75: Optional: set job-wide bash default to avoid repeating shell: bash.If you prefer DRY, set the job default shell instead of per-step.
test-package-rollup: name: "rollup v${{ matrix.rollup }} / node ${{ matrix.node }} / ${{ matrix.os }}" needs: - build runs-on: ${{ matrix.os }} + defaults: + run: + shell: bash
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/ci.yml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 150-150: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
108-126: Confirm Node 18 support policy.Node 18 is EOL; keep if you still support it, otherwise drop from matrix for faster CI.
vio
force-pushed
the
fix-commonjs-esm-output
branch
2 times, most recently
from
3b6c680 to
ae9791c
Compare
vio
force-pushed
the
fix-commonjs-esm-output
branch
from
ae9791c to
2bd2f06
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
94-96: Ensure bash for local registry script (rollup job)Required on Windows.
Apply:
- name: Setup local npm registry - run: ./scripts/setup-registry.sh + run: ./scripts/setup-registry.sh + shell: bash
♻️ Duplicate comments (5)
test/package/rollup/case-config.ts (1)
6-9: Rollup requires an explicit output.format.Without it,
bundle.generate(config.output)will fail. Use ES output for this ESM/TS case.export default defineConfig({ input: 'src/index.js', output: { dir: 'dist', + format: 'es', }, plugins: [webpackStatsPlugin({})], });test/package/rollup/case-config.js (1)
7-10: Specifyoutput.formatfor Rollup.Rollup won’t generate without a format. Using
eskeeps all three rollup test configs aligned and avoids size deltas in fixtures.module.exports = defineConfig({ input: 'src/index.js', output: { dir: 'dist', + format: 'es', }, plugins: [webpackStatsPlugin()], });.github/workflows/ci.yml (3)
112-112: Use the matrix OS for the vite jobSame issue here.
Apply:
- runs-on: ubuntu-latest + runs-on: ${{ matrix.os }}
145-147: Ensure bash for local registry script (vite job)Mirror the fix here.
Apply:
- name: Setup local npm registry - run: ./scripts/setup-registry.sh + run: ./scripts/setup-registry.sh + shell: bash
148-153: Bash shell for multiline install (vite job) and trim trailing spacesSame as rollup job.
Apply:
- name: Install test package dependencies - run: | - npm install + run: | + npm install npm install vite@${{ matrix.vite }} --force npx vite -v working-directory: test/package/vite + shell: bash
🧹 Nitpick comments (8)
tsconfig.base.json (1)
9-11: Align target/libs and trim permissive flags.
- Consider
target: "ES2022"to match Node 18 and yourlib: ["es2020","es2022.error"].- If TS sources only, move
allowJs: trueto the root tsconfig (IDE) and keep the build configs TS-only.- "target": "es2020", - "lib": ["es2020", "es2022.error"], + "target": "ES2022", + "lib": ["es2022", "es2022.error"], - "allowJs": true, + // consider enabling allowJs only in dev/IDE tsconfig.jsonAlso applies to: 14-17
test/package/rollup/case-config.ts (1)
2-2: Tiny consistency nit: unify plugin instantiation across configs.In .mjs you call
webpackStatsPlugin()with no args; here you pass{}. Pick one style for less diff churn.-import webpackStatsPlugin from 'rollup-plugin-webpack-stats'; +import webpackStatsPlugin from 'rollup-plugin-webpack-stats'; // Either: -plugins: [webpackStatsPlugin({})], +plugins: [webpackStatsPlugin()], // or pass real options in all three configstsconfig.json (1)
3-14: Optional: prefer NodeNext/Bundler resolution for ESM-first repos.Given
"module": "ESNext", consider"moduleResolution": "NodeNext"(Node 18 semantics) or"Bundler"if your tooling relies on export maps. Also, you can bumplibtoes2022to match the runtime.- "moduleResolution": "node", + "moduleResolution": "NodeNext", - "lib": ["es2020", "es2022.error"] + "lib": ["es2022", "es2022.error"]rollup.config.mjs (1)
26-26: Use a stable preserveModulesRootAbsolute paths here are fragile. Use 'src'.
Apply:
- preserveModulesRoot: CONTEXT, + preserveModulesRoot: PRESERVE_ROOT,Also applies to: 51-51
package.json (2)
14-16: Prefer "types" over "typings"Use the standard "types" field.
Apply:
- "typings": "lib/types/index.d.ts", + "types": "lib/types/index.d.ts",
49-51: Script name is misleadingbuild-type writes package.json "type" manifests, not TypeScript declarations. Rename for clarity.
Apply:
- "build": "npm run clean && rollup -c && npm run build-type", - "build-type": "echo '{\"type\":\"commonjs\"}' > lib/cjs/package.json && echo '{\"type\":\"module\"}' > lib/esm/package.json", + "build": "npm run clean && rollup -c && npm run postbuild:module-types", + "postbuild:module-types": "echo '{\"type\":\"commonjs\"}' > lib/cjs/package.json && echo '{\"type\":\"module\"}' > lib/esm/package.json",.github/workflows/ci.yml (2)
97-103: Bash shell for multiline install (rollup job) and trim trailing spacesWindows defaults to pwsh; use bash, and drop trailing spaces flagged by YAMLlint.
Apply:
- name: Install test package dependencies - run: | - npm install + run: | + npm install npm install rollup@${{ matrix.rollup }} --force npx rollup -v working-directory: test/package/rollup + shell: bash
86-88: "Install vitest" step likely redundantEach test package should bring its own devDeps. Consider removing or moving into the package fixture.
Also applies to: 137-139
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
.github/workflows/ci.yml(2 hunks).gitignore(1 hunks)package.json(5 hunks)rollup.config.mjs(1 hunks)test/package/rollup/case-config.js(1 hunks)test/package/rollup/case-config.mjs(1 hunks)test/package/rollup/case-config.ts(1 hunks)test/package/rollup/configs.test.ts(1 hunks)test/package/rollup/package.json(1 hunks)test/package/vite/case-config.js(1 hunks)test/package/vite/case-config.mjs(1 hunks)test/package/vite/case-config.ts(1 hunks)test/package/vite/case-options.mjs(1 hunks)test/package/vite/configs.test.ts(1 hunks)test/package/vite/options.test.ts(1 hunks)test/package/vite/package.json(1 hunks)tsconfig.base.json(1 hunks)tsconfig.cjs.json(1 hunks)tsconfig.esm.json(1 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- test/package/rollup/package.json
- test/package/vite/package.json
- test/package/vite/configs.test.ts
- test/package/vite/case-config.js
- tsconfig.cjs.json
- test/package/rollup/configs.test.ts
- test/package/vite/case-config.mjs
- test/package/vite/case-options.mjs
- .gitignore
- test/package/vite/case-config.ts
- test/package/vite/options.test.ts
- tsconfig.esm.json
🧰 Additional context used
🧬 Code graph analysis (2)
test/package/rollup/case-config.ts (1)
test/package/rollup/case-config.js (1)
webpackStatsPlugin(3-3)
test/package/rollup/case-config.mjs (1)
test/package/rollup/case-config.js (1)
webpackStatsPlugin(3-3)
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 150-150: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: publish
🔇 Additional comments (3)
tsconfig.json (1)
15-18: ts-node override looks good.Explicitly forcing CommonJS for ts-node reduces friction when the project is ESM-first.
package.json (2)
18-27: Exports rely on lib/esm/transform.js being builtMake sure rollup ESM input includes transform (see rollup.config.mjs fix). Otherwise consumers importing "./transform" will break.
Would you like a quick CI check that asserts lib/esm/transform.js and lib/cjs/transform.js exist after build?
71-88: You added node-externals; wire it uprollup-plugin-node-externals is listed but unused. See rollup.config.mjs suggestions to use nodeExternals().
| name: "rollup v${{ matrix.rollup }} / node ${{ matrix.node }} / ${{ matrix.os }}" | ||
| needs: | ||
| - build | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the matrix OS for the rollup job
Windows variants won’t run with runs-on hardcoded.
Apply:
- runs-on: ubuntu-latest
+ runs-on: ${{ matrix.os }}📝 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.
| runs-on: ubuntu-latest | |
| runs-on: ${{ matrix.os }} |
🤖 Prompt for AI Agents
In .github/workflows/ci.yml around line 58, the rollup job currently hardcodes
"runs-on: ubuntu-latest" which prevents Windows/macOS runs; change it to use the
job matrix by replacing runs-on: ubuntu-latest with runs-on: ${{ matrix.os }}
and add/ensure a strategy.matrix.os section for the job (e.g., include
windows-latest, ubuntu-latest, macos-latest or your supported OS list) so the
job runs across the matrix; adjust any OS-specific steps if needed.
| ".": { | ||
| "types": "./dist/index.d.ts", | ||
| "import": "./dist/index.mjs", | ||
| "require": "./dist/index.cjs" | ||
| "types": "./lib/types/index.d.ts", | ||
| "import": "./lib/esm/index.js", | ||
| "require": "./lib/cjs/index.js" | ||
| }, | ||
| "./transform": { | ||
| "types": "./dist/transform.d.ts", | ||
| "import": "./dist/transform.mjs", | ||
| "require": "./dist/transform.cjs" | ||
| "types": "./lib/types/transform.d.ts", | ||
| "import": "./lib/esm/transform.js", | ||
| "require": "./lib/cjs/transform.js" | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify declaration emit to lib/types
Ensure tsconfig sets declaration output once (to avoid duplicated emits) and to lib/types to satisfy exports/types.
Run:
Also applies to: 43-44, 49-51
🏁 Script executed:
#!/bin/bash
# Inspect tsconfigs for declaration settings
fd -a 'tsconfig*.json'
rg -nP '"declaration"\s*:\s*true' tsconfig*.json || true
rg -nP '"emitDeclarationOnly"\s*:\s*true' tsconfig*.json || true
rg -nP '"declarationDir"\s*:\s*"lib/types"' tsconfig*.json || true
# Show mapped export types
jq -r '.exports["."].types, .exports["./transform"].types, .types? // .typings?' package.jsonLength of output: 618
tsconfig.base.json: configure declaration output directory
Add under compilerOptions in tsconfig.base.json:
"emitDeclarationOnly": true,
"declarationDir": "lib/types"to emit all .d.ts files only into lib/types and prevent duplicate declaration files across your CJS/ESM builds.
🤖 Prompt for AI Agents
In package.json around lines 18 to 27, the project is emitting duplicate .d.ts
files into both CJS and ESM build outputs; update tsconfig.base.json under
compilerOptions to set "emitDeclarationOnly": true and "declarationDir":
"lib/types" so TypeScript emits only declaration files into lib/types
(preventing duplicates across lib/cjs and lib/esm) and ensure any build scripts
reference that declarationDir for packaging.
| import commonjsPlugin from '@rollup/plugin-commonjs'; | ||
| import nodeResolvePlugin from '@rollup/plugin-node-resolve'; | ||
| import typescriptPlugin from '@rollup/plugin-typescript'; |
There was a problem hiding this comment.
Externalization plugin is installed but unused
Regex /node_modules/ will not externalize bare imports (e.g., 'fs', 'rollup', deps). Use rollup-plugin-node-externals.
Apply:
import commonjsPlugin from '@rollup/plugin-commonjs';
import nodeResolvePlugin from '@rollup/plugin-node-resolve';
import typescriptPlugin from '@rollup/plugin-typescript';
+import { nodeExternals } from 'rollup-plugin-node-externals';Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In rollup.config.mjs around lines 3 to 5, the config imports commonjs,
node-resolve and typescript plugins but currently relies on a /node_modules/
regex (which doesn't externalize bare imports like 'fs' or package deps);
install and import rollup-plugin-node-externals, add it to the plugins array
before typescript/node-resolve, and configure it to externalize builtins and
dependencies (e.g., default options or { deps: true, peerDeps: true, builtins:
true }) so bare imports are treated as external modules; ensure plugin ordering
keeps node-externals early so CommonJS/node-resolve don't bundle those
externals.
| import nodeResolvePlugin from '@rollup/plugin-node-resolve'; | ||
| import typescriptPlugin from '@rollup/plugin-typescript'; | ||
|
|
||
| const CONTEXT = path.join(import.meta.dirname, './src'); |
There was a problem hiding this comment.
import.meta.dirname is undefined in Node ESM
This will resolve to "undefined/src". Use fileURLToPath to derive the directory or drop it entirely if you only need 'src' for preserveModulesRoot.
Apply:
+import { fileURLToPath } from 'node:url';
-const CONTEXT = path.join(import.meta.dirname, './src');
+// Use a relative preserve root instead of absolute path
+const PRESERVE_ROOT = 'src';Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In rollup.config.mjs around line 7, the expression
"path.join(import.meta.dirname, './src')" uses import.meta.dirname which is
undefined in Node ESM; replace this by deriving the directory from
import.meta.url with fileURLToPath and path.dirname (e.g., const __filename =
fileURLToPath(import.meta.url); const CONTEXT =
path.join(path.dirname(__filename), 'src')) or, if you only need the literal src
path for preserveModulesRoot, simply use path.resolve('src') /
path.join(process.cwd(), 'src') instead; update the import to include
fileURLToPath from 'url' if using import.meta.url.
|
|
||
| export default defineConfig([ | ||
| { | ||
| context: CONTEXT, |
There was a problem hiding this comment.
Misuse of Rollup "context"
context sets the top-level this binding, not a filesystem root. Remove it.
Apply:
- context: CONTEXT,
+ // no custom context neededDo the same in the second config object.
Also applies to: 43-43
🤖 Prompt for AI Agents
In rollup.config.mjs around lines 18 and 43, remove the incorrect "context:
CONTEXT" entries from both Rollup config objects (the "context" option sets the
top-level this binding, not a filesystem root). Delete those lines and, if you
intended to configure a project root or input/output directory, replace them
with the correct Rollup settings (e.g., set input/output.dir, adjust resolve
plugin options, or use a root-import plugin) rather than using the "context"
property.
| plugins: [ | ||
| nodeResolvePlugin({ | ||
| extensions: ['.js', '.cjs', '.json'], | ||
| }), | ||
| commonjsPlugin({ | ||
| defaultIsModuleExports: 'auto', | ||
| }), | ||
| typescriptPlugin({ | ||
| tsconfig: './tsconfig.cjs.json', | ||
| }), | ||
| ], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure externals are applied before resolving; add .ts to resolver
Place nodeExternals first and include '.ts' so extensionless TS imports resolve.
Apply:
- plugins: [
- nodeResolvePlugin({
- extensions: ['.js', '.cjs', '.json'],
- }),
- commonjsPlugin({
- defaultIsModuleExports: 'auto',
- }),
- typescriptPlugin({
- tsconfig: './tsconfig.cjs.json',
- }),
- ],
+ plugins: [
+ nodeExternals({ devDeps: false, deps: true, peerDeps: true, builtins: true }),
+ nodeResolvePlugin({
+ extensions: ['.ts', '.js', '.cjs', '.json'],
+ }),
+ commonjsPlugin({
+ defaultIsModuleExports: 'auto',
+ }),
+ typescriptPlugin({
+ tsconfig: './tsconfig.cjs.json',
+ }),
+ ],📝 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.
| plugins: [ | |
| nodeResolvePlugin({ | |
| extensions: ['.js', '.cjs', '.json'], | |
| }), | |
| commonjsPlugin({ | |
| defaultIsModuleExports: 'auto', | |
| }), | |
| typescriptPlugin({ | |
| tsconfig: './tsconfig.cjs.json', | |
| }), | |
| ], | |
| plugins: [ | |
| nodeExternals({ devDeps: false, deps: true, peerDeps: true, builtins: true }), | |
| nodeResolvePlugin({ | |
| extensions: ['.ts', '.js', '.cjs', '.json'], | |
| }), | |
| commonjsPlugin({ | |
| defaultIsModuleExports: 'auto', | |
| }), | |
| typescriptPlugin({ | |
| tsconfig: './tsconfig.cjs.json', | |
| }), | |
| ], |
🤖 Prompt for AI Agents
In rollup.config.mjs around lines 30 to 40, the plugins order and resolver
extensions are wrong: move the nodeExternals plugin to be the first plugin in
the array so externals are applied before module resolution, and update the
nodeResolvePlugin extensions to include '.ts' (e.g. ['.ts', '.js', '.cjs',
'.json']) so extensionless TypeScript imports resolve correctly; keep the rest
of the plugins in the same relative order after these changes.
| plugins: [ | ||
| nodeResolvePlugin({ | ||
| extensions: ['.js', '.mjs', '.cjs', '.json'], | ||
| }), | ||
| commonjsPlugin(), | ||
| typescriptPlugin({ | ||
| tsconfig: './tsconfig.esm.json', | ||
| }), | ||
| ], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror resolver and externals in ESM build
Keep plugin order consistent and include '.ts'.
Apply:
- plugins: [
- nodeResolvePlugin({
- extensions: ['.js', '.mjs', '.cjs', '.json'],
- }),
- commonjsPlugin(),
- typescriptPlugin({
- tsconfig: './tsconfig.esm.json',
- }),
- ],
+ plugins: [
+ nodeExternals({ devDeps: false, deps: true, peerDeps: true, builtins: true }),
+ nodeResolvePlugin({
+ extensions: ['.ts', '.js', '.mjs', '.cjs', '.json'],
+ }),
+ commonjsPlugin(),
+ typescriptPlugin({
+ tsconfig: './tsconfig.esm.json',
+ }),
+ ],📝 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.
| plugins: [ | |
| nodeResolvePlugin({ | |
| extensions: ['.js', '.mjs', '.cjs', '.json'], | |
| }), | |
| commonjsPlugin(), | |
| typescriptPlugin({ | |
| tsconfig: './tsconfig.esm.json', | |
| }), | |
| ], | |
| plugins: [ | |
| nodeExternals({ devDeps: false, deps: true, peerDeps: true, builtins: true }), | |
| nodeResolvePlugin({ | |
| extensions: ['.ts', '.js', '.mjs', '.cjs', '.json'], | |
| }), | |
| commonjsPlugin(), | |
| typescriptPlugin({ | |
| tsconfig: './tsconfig.esm.json', | |
| }), | |
| ], |
🤖 Prompt for AI Agents
In rollup.config.mjs around lines 55 to 63, the ESM build plugins should mirror
the resolver and externals configuration used elsewhere and include TypeScript
files: update the nodeResolvePlugin extensions array to add '.ts' (['.js',
'.mjs', '.cjs', '.json', '.ts']), and ensure the plugin order matches the other
build configuration (place nodeResolvePlugin before commonjsPlugin and
typescriptPlugin and include any externals/resolver plugin calls used in the
other build so the ESM bundle resolves externals consistently).
| output: { | ||
| dir: 'dist', | ||
| }, | ||
| plugins: [webpackStatsPlugin()], |
There was a problem hiding this comment.
Add output.format to prevent Rollup generate errors.
Use ESM here for parity with the TS config.
export default defineConfig({
input: 'src/index.js',
output: {
dir: 'dist',
+ format: 'es',
},
plugins: [webpackStatsPlugin()],
});📝 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.
| output: { | |
| dir: 'dist', | |
| }, | |
| plugins: [webpackStatsPlugin()], | |
| export default defineConfig({ | |
| input: 'src/index.js', | |
| output: { | |
| dir: 'dist', | |
| format: 'es', | |
| }, | |
| plugins: [webpackStatsPlugin()], | |
| }); |
🤖 Prompt for AI Agents
In test/package/rollup/case-config.mjs around lines 6 to 9, Rollup's output
block is missing the required format field which causes generate errors; update
the output config to include output.format set to 'es' (ESM) so Rollup emits ESM
output consistent with the TS config, leaving the plugins array unchanged.
tsconfig.base.json
Outdated
| "declaration": true, | ||
| "sourceMap": true, | ||
| "noImplicitReturns": true, | ||
| "noImplicitOverride": true, | ||
| "noUnusedLocals": true, | ||
| "noUnusedParameters": true, | ||
| "skipLibCheck": true, | ||
| "forceConsistentCasingInFileNames": true, | ||
| "noEmit": true |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Types won’t emit: noEmit: true conflicts with declaration and declarationDir.
If this base config is used for your “build-type” step, nothing will be written. Either enable emit here or move declaration settings to a dedicated tsconfig for types.
Option A (emit only d.ts from this config):
"declaration": true,
- "sourceMap": true,
+ "declarationMap": true,
"noImplicitReturns": true,
"noImplicitOverride": true,
"noUnusedLocals": true,
"noUnusedParameters": true,
"skipLibCheck": true,
"forceConsistentCasingInFileNames": true,
- "noEmit": true
+ "noEmit": false,
+ "emitDeclarationOnly": trueOption B (keep base as “typecheck-only” and emit types elsewhere):
- "declarationDir": "./lib/types",
+ // keep paths/types out of the base to avoid confusion; set them in tsconfig.types.json
...
- "declaration": true,
+ // declarations configured in tsconfig.types.json
...
- "noEmit": true
+ "noEmit": trueIf you go with Option B, add a tsconfig.types.json that extends this base and sets:
- noEmit: false
- declaration: true
- emitDeclarationOnly: true
- declarationDir: ./lib/types
📝 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.
| "declaration": true, | |
| "sourceMap": true, | |
| "noImplicitReturns": true, | |
| "noImplicitOverride": true, | |
| "noUnusedLocals": true, | |
| "noUnusedParameters": true, | |
| "skipLibCheck": true, | |
| "forceConsistentCasingInFileNames": true, | |
| "noEmit": true | |
| "declaration": true, | |
| "declarationMap": true, | |
| "noImplicitReturns": true, | |
| "noImplicitOverride": true, | |
| "noUnusedLocals": true, | |
| "noUnusedParameters": true, | |
| "skipLibCheck": true, | |
| "forceConsistentCasingInFileNames": true, | |
| "noEmit": false, | |
| "emitDeclarationOnly": true |
🤖 Prompt for AI Agents
In tsconfig.base.json around lines 18 to 26, the current settings enable
declarations but also set "noEmit": true which prevents any output; fix by
either enabling emit in this base config (set "noEmit": false and ensure
"declaration" and "declarationDir" are correct for your build) or keep the base
as a typecheck-only config and create a new tsconfig.types.json that extends
this base and overrides to "noEmit": false, "declaration": true,
"emitDeclarationOnly": true and "declarationDir": "./lib/types"; pick one option
and remove conflicting flags so declarations are actually emitted where
intended.
a65cb3a to
fd6e4cc
Compare
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 (1)
.github/workflows/ci.yml (1)
95-103: Windows: force bash for the registry script and multi-line installs (Rollup job)Without shell: bash, ./scripts/setup-registry.sh fails on windows-latest.
Apply:
- name: Setup local npm registry - run: ./scripts/setup-registry.sh + run: ./scripts/setup-registry.sh + shell: bash - name: Install test package dependencies run: | - npm install + npm install npm install rollup@${{ matrix.rollup }} --force npx rollup -v working-directory: test/package/rollup + shell: bash
♻️ Duplicate comments (6)
rollup.config.mjs (3)
7-7: Replace import.meta.dirname and use a relative preserve root ('src')import.meta.dirname is undefined in Node ESM; and preserveModulesRoot should not rely on an absolute path. Use a simple relative root and drop CONTEXT usage here.
Apply:
-const CONTEXT = path.join(import.meta.dirname, './src'); +const PRESERVE_ROOT = 'src';Then:
- preserveModulesRoot: CONTEXT, + preserveModulesRoot: PRESERVE_ROOT,(and same for the ESM output).
Also applies to: 25-27, 50-53
18-18: Remove misuse of Rollup “context” optioncontext controls the top-level this binding, not filesystem roots. It’s unnecessary here.
Apply:
- context: CONTEXT, + // no custom contextAlso applies to: 43-43
29-29: Externalize deps/peerDeps/builtins via node-externals; ensure TS resolution and correct plugin orderThe /node_modules/ regex won’t catch bare IDs; place nodeExternals first, include '.ts' in resolver, and keep resolve → commonjs → typescript order.
Apply:
+import { nodeExternals } from 'rollup-plugin-node-externals'; export default defineConfig([ { - external: /node_modules/, plugins: [ + nodeExternals({ deps: true, peerDeps: true, devDeps: false, builtins: true }), nodeResolvePlugin({ - extensions: ['.js', '.cjs', '.json'], + extensions: ['.ts', '.js', '.cjs', '.json'], }), commonjsPlugin({ defaultIsModuleExports: 'auto', }), typescriptPlugin({ tsconfig: './tsconfig.cjs.json', }), ], }, { - external: /node_modules/, plugins: [ + nodeExternals({ deps: true, peerDeps: true, devDeps: false, builtins: true }), nodeResolvePlugin({ - extensions: ['.js', '.mjs', '.cjs', '.json'], + extensions: ['.ts', '.js', '.mjs', '.cjs', '.json'], }), - commonjsPlugin(), + commonjsPlugin({ + defaultIsModuleExports: 'auto', + }), typescriptPlugin({ tsconfig: './tsconfig.esm.json', }), ], }, ]);Also applies to: 54-54, 30-40, 55-63, 3-5
package.json (1)
71-87: node-externals is added but not used in rollup.configPlease wire it in (see rollup.config.mjs comment).
Run to confirm usage:
#!/bin/bash rg -n "node-externals|nodeExternals" rollup.config.mjs -n -C1 || true.github/workflows/ci.yml (2)
54-59: Use the matrix OS for the Rollup jobCurrently pinned to ubuntu; windows variants won’t run.
Apply:
- runs-on: ubuntu-latest + runs-on: ${{ matrix.os }}
108-113: Use the matrix OS for the Vite jobSame issue as the Rollup job.
Apply:
- runs-on: ubuntu-latest + runs-on: ${{ matrix.os }}
🧹 Nitpick comments (4)
rollup.config.mjs (1)
1-1: Drop unused 'path' import after removing CONTEXTIf you adopt PRESERVE_ROOT, path is no longer used.
Apply:
-import path from 'node:path';package.json (2)
49-51: Consider adding a 'prepack' (or 'prepare') hook to ensure builds when publishing locallyOptional but helps outside CI.
Apply:
"scripts": { - "build": "npm run clean && rollup -c && npm run build-package-type", + "build": "npm run clean && rollup -c && npm run build-package-type", + "prepack": "npm run build", "build-package-type": "echo '{\"type\":\"module\"}' > lib/esm/package.json",
16-16: Prefer 'types' over 'typings' (keep both for compatibility)TypeScript recognizes both, but 'types' is the canonical field.
Apply:
- "typings": "lib/types/index.d.ts", + "types": "lib/types/index.d.ts", + "typings": "lib/types/index.d.ts",.github/workflows/ci.yml (1)
52-52: Artifact path: prefer a deterministic globlib*/ may be brittle; lib/** is clearer and includes nested files.
Apply:
- path: lib*/ + path: lib/**
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.github/workflows/ci.yml(2 hunks).gitignore(1 hunks)package.json(5 hunks)rollup.config.mjs(1 hunks)test/package/rollup/package.json(1 hunks)tsconfig.base.json(1 hunks)tsconfig.cjs.json(1 hunks)tsconfig.esm.json(1 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/package/rollup/package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- tsconfig.cjs.json
- tsconfig.json
- tsconfig.esm.json
- .gitignore
- tsconfig.base.json
🧰 Additional context used
🧬 Code graph analysis (1)
rollup.config.mjs (1)
src/plugin.ts (1)
generateBundle(31-59)
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 150-150: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: vite v5 / node 22 / windows-latest
🔇 Additional comments (6)
rollup.config.mjs (2)
9-12: Good: shared INPUT covers both 'index' and 'transform' entriesThis aligns outputs with package.json exports for "./" and "./transform".
14-14: Good: OUTPUT_DIR switched to 'lib'Matches the new publish layout and CI artifact path.
package.json (3)
14-27: Exports and entry points correctly target lib/cjs, lib/esm, and lib/typesThe mapping for both main entry and "./transform" looks consistent with the build outputs.
43-44: Files whitelist is finePublishing only lib/ keeps the package lean and aligned with build artifacts.
18-27: The script above will show where each tsconfig is outputting compiled files and whether declaration-only emit is configured per build. Once we confirm if CJS/ESM builds both emit declarations into their own outDirs (thus duplicating .d.ts) versus emitting only into lib/types viaemitDeclarationOnlyanddeclarationDir, we can finalize..github/workflows/ci.yml (1)
159-165: Good: tests job depends on both package test matricesEnsures overall status reflects both Rollup and Vite runs.
| - name: Setup local npm registry | ||
| run: ./scripts/setup-registry.sh | ||
|
|
||
| - name: Install test package dependencies | ||
| run: | | ||
| npm install | ||
| npm install vite@${{ matrix.vite }} --force | ||
| npx vite -v | ||
| working-directory: test/package/vite |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Windows: force bash for the registry script and installs (Vite job) + trim trailing space
Prevents failures on windows and fixes YAML lint trailing spaces.
Apply:
- name: Setup local npm registry
- run: ./scripts/setup-registry.sh
+ run: ./scripts/setup-registry.sh
+ shell: bash
- name: Install test package dependencies
run: |
- npm install
+ npm install
npm install vite@${{ matrix.vite }} --force
npx vite -v
working-directory: test/package/vite
+ shell: bash📝 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.
| - name: Setup local npm registry | |
| run: ./scripts/setup-registry.sh | |
| - name: Install test package dependencies | |
| run: | | |
| npm install | |
| npm install vite@${{ matrix.vite }} --force | |
| npx vite -v | |
| working-directory: test/package/vite | |
| - name: Setup local npm registry | |
| run: ./scripts/setup-registry.sh | |
| shell: bash | |
| - name: Install test package dependencies | |
| run: | | |
| npm install | |
| npm install vite@${{ matrix.vite }} --force | |
| npx vite -v | |
| working-directory: test/package/vite | |
| shell: bash |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 150-150: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
.github/workflows/ci.yml lines 145-153: the workflow step runs a shell script
and multiline npm commands which can fail on Windows and contains a trailing
space after "npm install "; change the steps to force bash for the registry
script and the multiline install block (e.g., use "bash
./scripts/setup-registry.sh" and run the multiline commands under bash with
"bash -lc") and remove the trailing space after "npm install" so the YAML is
lint-clean and the commands run reliably on Windows.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
tsconfig.base.json (1)
12-24: Base is typecheck-only; previous conflict is resolved.Good: no
declaration*settings here withnoEmit: true. Keep it as the strict base for editors/CI.rollup.config.mjs (7)
3-5: Add node-externals and wire it upYou added the dep but never import/use it. Import it so we can externalize deps/peerDeps/builtins.
import commonjsPlugin from '@rollup/plugin-commonjs'; import nodeResolvePlugin from '@rollup/plugin-node-resolve'; import typescriptPlugin from '@rollup/plugin-typescript'; +import { nodeExternals } from 'rollup-plugin-node-externals';
7-7: import.meta.dirname is undefined in Node ESMReplace with a simple literal preserve root; no need for filesystem math here.
-const CONTEXT = path.join(import.meta.dirname, './src'); +const PRESERVE_ROOT = 'src';
18-18: Remove misuse of Rollup “context”context sets top-level this, not a project root. Safe to drop.
- context: CONTEXT, + // no custom contextApply in both configs.
Also applies to: 43-43
25-27: Use stable preserveModulesRootPoint to the folder name, not an absolute path.
- preserveModulesRoot: CONTEXT, + preserveModulesRoot: PRESERVE_ROOT,Apply in both configs.
Also applies to: 50-52
29-29: /node_modules/ regex doesn’t externalize bare IDsBare imports and builtins won’t match. Use nodeExternals and drop this.
- external: /node_modules/, + // externals handled by nodeExternals()Apply in both configs.
Also applies to: 54-54
31-40: Plugin order + resolver extensionsExternalize first and resolve .ts. Keep CJS behavior the same otherwise.
- plugins: [ - nodeResolvePlugin({ - extensions: ['.js', '.cjs', '.json'], - }), - commonjsPlugin({ - defaultIsModuleExports: 'auto', - }), - typescriptPlugin({ - tsconfig: './tsconfig.lib-cjs.json', - }), - ], + plugins: [ + nodeExternals({ devDeps: false, deps: true, peerDeps: true, builtins: true }), + nodeResolvePlugin({ + extensions: ['.ts', '.js', '.cjs', '.json'], + }), + commonjsPlugin({ + defaultIsModuleExports: 'auto', + }), + typescriptPlugin({ + tsconfig: './tsconfig.lib-cjs.json', + }), + ],
56-63: Mirror externals and .ts resolution in ESM buildAlign with CJS config for consistency.
- plugins: [ - nodeResolvePlugin({ - extensions: ['.js', '.mjs', '.cjs', '.json'], - }), - commonjsPlugin(), - typescriptPlugin({ - tsconfig: './tsconfig.lib-esm.json', - }), - ], + plugins: [ + nodeExternals({ devDeps: false, deps: true, peerDeps: true, builtins: true }), + nodeResolvePlugin({ + extensions: ['.ts', '.js', '.mjs', '.cjs', '.json'], + }), + commonjsPlugin(), + typescriptPlugin({ + tsconfig: './tsconfig.lib-esm.json', + }), + ],
🧹 Nitpick comments (6)
tsconfig.lib.json (1)
16-17: Consider disablingallowJsfor the library build.Keeps the lib TS-only and avoids accidentally including JS test/helpers in declaration emit.
- "allowJs": true, + "allowJs": false,tsconfig.base.json (1)
8-11: Optional: align module settings across configs for consistency.If you rely on NodeNext semantics repo-wide, consider documenting that base omits module settings intentionally (typecheck-only) to avoid confusion with lib configs.
tsconfig.json (1)
14-17: Consider adding test type roots if Vitest globals are used.If tests use Vitest globals, add
"types": ["node", "vitest/globals"]in a test-specific tsconfig or here.package.json (3)
14-16: Prefer “types” over “typings”Modern field name is "types". Keep exports.types as-is.
- "typings": "lib/types/index.d.ts", + "types": "lib/types/index.d.ts",
49-51: Optional: add prepublishOnly safety netEnsures types/outputs exist when publishing from a fresh clone.
"scripts": { "build": "npm run clean && rollup -c && npm run build-package-type", "build-package-type": "echo '{\"type\":\"module\"}' > lib/esm/package.json", "clean": "rimraf ./lib", + "prepublishOnly": "npm run build",
93-96: Consider marking rolldown optional tooIf supported but not required at runtime, mirror vite/rollup optional meta.
"peerDependenciesMeta": { + "rolldown": { + "optional": true + }, "vite": { "optional": true },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
package.json(5 hunks)rollup.config.mjs(1 hunks)tsconfig.base.json(1 hunks)tsconfig.json(1 hunks)tsconfig.lib-cjs.json(1 hunks)tsconfig.lib-esm.json(1 hunks)tsconfig.lib.json(1 hunks)
🔇 Additional comments (6)
tsconfig.lib-cjs.json (1)
3-5: This config won’t emit JS (inheritsnoEmit: true)—is that intentional?If Rollup handles JS output for CJS, this is fine. Otherwise, override
noEmithere. Please confirm the build path for CJS."compilerOptions": { "outDir": "./lib/cjs" + // If you expect tsc to write JS for CJS, also add: + // "noEmit": false }tsconfig.json (1)
7-11: Runtime ts-node CommonJS override looks good with NodeNext.The NodeNext setup plus ts-node CJS override is a common pattern and should work for scripts.
rollup.config.mjs (2)
14-14: LGTM: output directory switched to libMatches package.json files/exports.
22-24: LGTM: dual formats and entry file namescjs/ and esm/ subfolders are clear and align with exports mapping.
Also applies to: 47-49
package.json (2)
71-75: LGTM: devDeps include resolve/commonjs/externalsMatches rollup.config intent.
Also applies to: 84-85
90-91: LGTM: plugin version aligned with testsrollup-plugin-stats beta matches the transform export wiring.
| "types": "./lib/types/index.d.ts", | ||
| "import": "./lib/esm/index.js", | ||
| "require": "./lib/cjs/index.js" | ||
| }, | ||
| "./transform": { | ||
| "types": "./dist/transform.d.ts", | ||
| "import": "./dist/transform.mjs", | ||
| "require": "./dist/transform.cjs" | ||
| "types": "./lib/types/transform.d.ts", | ||
| "import": "./lib/esm/transform.js", | ||
| "require": "./lib/cjs/transform.js" | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify .d.ts emit goes to lib/types exactly once
Ensure tsconfig emits declarations only to lib/types to satisfy these exports and avoid duplicates across CJS/ESM.
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tsconfig files:" && fd -a 'tsconfig*.json' || true
echo
echo 'Declaration settings:'
rg -nP '"declaration"\s*:\s*true' tsconfig*.json || true
rg -nP '"emitDeclarationOnly"\s*:\s*true' tsconfig*.json || true
rg -nP '"declarationDir"\s*:\s*"lib/types"' tsconfig*.json || true
echo
echo 'Package types mapping:'
jq -r '.exports["."].types, .exports["./transform"].types, .types? // .typings?' package.jsonLength of output: 882
Configure TypeScript to emit declarations only to lib/types
In your declaration-producing tsconfig (e.g. tsconfig.lib-esm.json or a shared base), add
"emitDeclarationOnly": true,
"declarationDir": "lib/types"and turn off declaration in other build configs so .d.ts files aren’t duplicated under lib/esm or lib/cjs.
🤖 Prompt for AI Agents
In package.json around lines 19 to 27, the package points to .d.ts files under
lib/types but your TypeScript build is currently producing declaration files
into multiple output folders; modify your declaration-producing tsconfig (e.g.
tsconfig.lib-esm.json or shared base) to set emitDeclarationOnly: true and
declarationDir: "lib/types", and ensure other tsconfig build configs have
declaration disabled so .d.ts files are not duplicated into lib/esm or lib/cjs
during the build.
| "compilerOptions": { | ||
| "outDir": "./lib/esm", | ||
| "declarationDir": "./lib/types", | ||
| "declaration": true | ||
| } |
There was a problem hiding this comment.
Fix ESM types emission: override noEmit and emit only declarations.
Currently inherits noEmit: true from tsconfig.lib.json, so no .d.ts will be written despite declaration: true.
"compilerOptions": {
"outDir": "./lib/esm",
"declarationDir": "./lib/types",
- "declaration": true
+ "declaration": true,
+ "emitDeclarationOnly": true,
+ "noEmit": false,
+ "declarationMap": true
}📝 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.
| "compilerOptions": { | |
| "outDir": "./lib/esm", | |
| "declarationDir": "./lib/types", | |
| "declaration": true | |
| } | |
| "compilerOptions": { | |
| "outDir": "./lib/esm", | |
| "declarationDir": "./lib/types", | |
| "declaration": true, | |
| "emitDeclarationOnly": true, | |
| "noEmit": false, | |
| "declarationMap": true | |
| } |
🤖 Prompt for AI Agents
In tsconfig.lib-esm.json around lines 3 to 7, the config inherits noEmit: true
from tsconfig.lib.json so declaration files are not emitted despite declaration:
true; override the inheritance by adding "noEmit": false and set
"emitDeclarationOnly": true alongside "declaration": true so tsc emits only
.d.ts files into the declarationDir (./lib/types).
| /* behaviour options */ | ||
| "strict": true, | ||
| "allowJs": true, | ||
| "allowSyntheticDefaultImports": true, | ||
| "noFallthroughCasesInSwitch": true, | ||
| "sourceMap": true, | ||
| "noImplicitReturns": true, | ||
| "noImplicitOverride": true, | ||
| "noUnusedLocals": true, | ||
| "noUnusedParameters": true, | ||
| "skipLibCheck": true, | ||
| "forceConsistentCasingInFileNames": true, | ||
| "noEmit": true | ||
| }, |
There was a problem hiding this comment.
Types won’t emit downstream unless children override noEmit: true.
This base lib config sets noEmit: true, but tsconfig.lib-esm.json enables declaration without turning emit back on. Ensure the ESM config explicitly sets noEmit: false (and preferably emitDeclarationOnly: true) so .d.ts files are produced.
Apply in tsconfig.lib-esm.json (see that file’s comment for exact diff).
🤖 Prompt for AI Agents
tsconfig.lib.json around lines 14-27: the base config sets "noEmit": true which
prevents downstream projects from emitting files; update tsconfig.lib-esm.json
to explicitly override this by setting "noEmit": false and add
"emitDeclarationOnly": true (while keeping "declaration": true) so only .d.ts
files are emitted for the ESM build.
de6dd3e to
42e546e
Compare
Summary by CodeRabbit
Chores
Tests
CI