Skip to content

Conversation

@vio
Copy link
Member

@vio vio commented Aug 27, 2025

Summary by CodeRabbit

  • Refactor
    • Build output and packaging reorganized to publish from lib with separate CJS and ESM bundles; exports and typings updated accordingly.
  • Tests
    • New integration tests validate multiple build variants (CJS/ESM/TS) for Rollup and Vite; standardized option tests across packages.
  • Chores
    • CI and ignore rules updated for the new build layout; test scripts and dev tooling enhanced (TypeScript, build/test scripts, and related dev deps).
  • Release
    • Version bumped to a beta release.

@coderabbitai
Copy link

coderabbitai bot commented Aug 27, 2025

Walkthrough

This PR reorganizes build outputs from dist to lib, replacing a single Rollup config with separate CJS/ESM builds and adding tsconfig.lib-*.json files. package.json and CI were updated to reflect lib-based outputs, new build/type-check/clean scripts, and additional devDependencies (rollup/Vite/TypeScript tooling). It adds per-package test harnesses and Vitest integration tests for Rollup, Vite, and Rolldown (including CJS/ESM/TS build scripts), introduces many case-config/case-options files, updates .gitignore entries, and adjusts unit tests to use typed fixtures from rollup’s OutputBundle.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60–90 minutes

Possibly related PRs

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title indicates adding Rollup test cases for commonjs, mjs, and ts configs, which aligns with the new integration tests added under test/package/rollup. Although the pull request includes broader changes such as Vite test adjustments and packaging configuration updates, the title correctly identifies a real and significant aspect of the changeset. Because it is directly related to actual changes and not misleading, it satisfies the partially related criterion.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-commonjs-esm-output

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1361912 and c311958.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (1)
  • package.json (4 hunks)
🔇 Additional comments (1)
package.json (1)

50-50: Fix build-esm-packages for Windows compatibility

build-esm-packages still relies on echo with single quotes and shell redirection, which breaks on Windows (CMD/PowerShell treat single quotes literally). That leaves contributors on Windows unable to run npm run build. Please convert this to a small Node script so it works cross-platform and ensures the target directories exist.

-    "build-esm-packages": "echo '{\"type\":\"module\"}' > lib/esm/package.json",
+    "build-esm-packages": "node -e \"const fs=require('fs');fs.mkdirSync('lib/cjs',{recursive:true});fs.mkdirSync('lib/esm',{recursive:true});fs.writeFileSync('lib/cjs/package.json',JSON.stringify({type:'commonjs'}));fs.writeFileSync('lib/esm/package.json',JSON.stringify({type:'module'}));\"",

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@socket-security
Copy link

socket-security bot commented Aug 27, 2025

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Added@​types/​deep-freeze-strict@​1.1.21001005477100
Addedrimraf@​6.0.19910010078100
Addedcross-env@​7.0.310010010079100
Added@​rollup/​plugin-node-resolve@​16.0.19910010084100
Added@​rollup/​plugin-commonjs@​28.0.69910010086100

View full report

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (5)
test/package/rollup/package.json (1)

15-16: Do not remove cross-env; ts-node can be dropped

cross-env is referenced in tests (configs.test.ts line 16), so it must stay. No evidence of ts-node usage outside of package.json/package-lock.json, so you can safely remove ts-node from devDependencies.

• test/package/rollup/configs.test.ts:

  • Line 16: await exec(\cross-env NO_COLOR=true ${testCase.run}`);usescross-env. • No other occurrences of ts-node` in scripts/tests (only in package.json and lock file).

Revised devDependencies diff:

   "devDependencies": {
     "@rollup/plugin-typescript": "^12.1.4",
-    "cross-env": "^7.0.3",
+    "cross-env": "^7.0.3",
     "rollup": "^4.9.1",
     "rollup-plugin-stats": "*",
-    "ts-node": "^10.9.2",
     "typescript": "^5.9.2"
   }
test/package/rollup/options.test.ts (1)

14-58: Close Rollup bundles to avoid fd leaks and flakiness.

Add await bundle.close() in each test after bundle.generate(...) (or after assertions).

Apply this diff:

   test('should output stats JSON file', async () => {
     const bundle = await rollup(config);
     await bundle.generate(config.output);
@@
     expect(stats['index.js']).toMatchObject({
       fileName: 'index.js',
     });
+    await bundle.close();
   });
@@
   test('should output stats JSON file with explicit compilation file name', async () => {
     const bundle = await rollup(dynamicOptions);
     await bundle.generate(dynamicOptions.output);
@@
     expect(stats['index.js']).toMatchObject({
       fileName: 'index.js',
     });
+    await bundle.close();
   });
@@
   test('should output stats JSON file with custom relative filename', async () => {
     const bundle = await rollup(relativeFileNameConfig);
     await bundle.generate(relativeFileNameConfig.output);
@@
     expect(stats['index.js']).toMatchObject({
       fileName: 'index.js',
     });
+    await bundle.close();
   });
@@
   test('should output stats JSON file with custom absolute filename', async () => {
     const bundle = await rollup(absoluteFileNameConfig);
     await bundle.generate(absoluteFileNameConfig.output);
@@
     expect(stats['index.js']).toMatchObject({
       fileName: 'index.js',
     });
+    await bundle.close();
   });
test/package/rollup/configs.test.ts (1)

13-22: Table-driven tests and robust output matching.

Use test.each and match against combined stdout + stderr to avoid CLI stream differences.

Apply this diff:

-describe('rollup package - configs', () => {
-  testCases.forEach((testCase) => {
-    test(`should build successfully with rollup ${testCase.type} config`, async () => {
-      const { stderr } = await exec(`cross-env NO_COLOR=true ${testCase.run}`);
-
-      expect(stderr).toMatch('Stats saved to dist');
-      expect(stderr).toMatch('created dist in');
-    });
-  });
-});
+describe('rollup package - configs', () => {
+  test.each(testCases)('should build successfully with rollup %s config', async ({ type, run }) => {
+    const { stdout, stderr } = await exec(`cross-env NO_COLOR=true ${run}`);
+    const out = `${stdout}\n${stderr}`;
+    expect(out).toMatch('Stats saved to dist');
+    expect(out).toMatch('created dist in');
+  });
+});
test/package/rollup/rollup-options.config.mjs (2)

41-43: Make absolute path portable across OSes.

/tmp/... is Unix-specific. Use os.tmpdir() (and path.join) to work on Windows/macOS/Linux.

Apply this diff:

   plugins: [
     stats({
-      fileName: '/tmp/custom-stats.json',
+      fileName: path.join(os.tmpdir(), 'custom-stats.json'),
     }),
   ],

Add these imports at the top of the file:

import os from 'node:os';
import path from 'node:path';

1-10: Minor: reduce duplication with a base config factory.

If you touch this again, consider a small helper to DRY up input, output.dir, and plugins scaffolding across variants.

Also applies to: 12-45

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 7f69a04 and 581680f.

⛔ Files ignored due to path filters (1)
  • test/package/rollup/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (8)
  • test/package/rollup/artifacts/stats.json (1 hunks)
  • test/package/rollup/configs.test.ts (1 hunks)
  • test/package/rollup/options.test.ts (1 hunks)
  • test/package/rollup/package.json (1 hunks)
  • test/package/rollup/rollup-options.config.mjs (1 hunks)
  • test/package/rollup/rollup.config.js (1 hunks)
  • test/package/rollup/rollup.config.mjs (1 hunks)
  • test/package/rollup/rollup.config.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
test/package/rollup/rollup.config.mjs (2)
test/package/rollup/rollup.config.js (1)
  • stats (2-2)
src/index.ts (1)
  • rollupStats (34-59)
test/package/rollup/rollup.config.ts (2)
test/package/rollup/rollup.config.js (1)
  • stats (2-2)
src/index.ts (1)
  • rollupStats (34-59)
test/package/rollup/rollup-options.config.mjs (2)
test/package/rollup/rollup.config.js (1)
  • stats (2-2)
src/index.ts (1)
  • rollupStats (34-59)
test/package/rollup/package.json (1)
src/index.ts (1)
  • rollupStats (34-59)
test/package/rollup/rollup.config.js (1)
src/index.ts (1)
  • rollupStats (34-59)
🪛 GitHub Check: lint
test/package/rollup/rollup.config.js

[failure] 2-2:
A require() style import is forbidden


[failure] 1-1:
A require() style import is forbidden

🪛 GitHub Actions: ci
test/package/rollup/rollup.config.js

[error] 1-1: ESLint error: A require() style import is forbidden. (@typescript-eslint/no-require-imports). Command: npm run lint

🔇 Additional comments (5)
test/package/rollup/package.json (1)

9-9: Rollup CLI TS config flag is correct

Per the official Rollup v4 documentation, to use a TypeScript configuration file you should install @rollup/plugin-typescript and invoke Rollup with:

rollup --config rollup.config.ts --configPlugin typescript

– the --configPlugin typescript flag is exactly the officially recommended approach for loading a rollup.config.ts file (rollupjs.org).

No changes are required here.

test/package/rollup/rollup.config.ts (1)

1-10: LGTM for TS config

The minimal TS config with defineConfig and stats() is correct and mirrors other variants.

test/package/rollup/rollup.config.mjs (1)

1-10: LGTM for ESM config

Consistent with CJS/TS variants; uses stats() as intended.

test/package/rollup/rollup.config.js (1)

4-10: LGTM after lint fix

Config is minimal and mirrors the ESM/TS variants; good for exercising the CJS path.

test/package/rollup/options.test.ts (1)

7-7: Import switch to rollup-options.config.mjs looks correct.

Default and named imports match the new module’s exports. No issues.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (4)
test/package/vite/vite.config.ts (1)

6-11: Replace non-standard top-level output with build.outDir

Vite ignores a top-level output; use build.outDir instead.

-  output: {
-    dir: 'dist',
-  },
-  build: {
+  build: {
+    outDir: 'dist',
     rollupOptions: {
test/package/vite/configs.test.ts (1)

13-13: Suite name says “rollup” but this file tests Vite

Tweak the descriptor for clarity.

-describe('rollup package - configs', () => {
+describe('vite package - configs', () => {
test/package/vite/vite.config.mjs (1)

6-11: Use build.outDir instead of top-level output

Align with Vite’s schema.

-  output: {
-    dir: 'dist',
-  },
-  build: {
+  build: {
+    outDir: 'dist',
     rollupOptions: {
test/package/vite/vite.config.js (1)

5-20: Clarify non-standard top-level output and set Vite’s build.outDir

output at the root isn’t a Vite option; it’s test-only metadata. Keep it if tests read config.output.dir, but also set build.outDir for correctness and add a note to avoid confusion.

 module.exports = defineConfig({
   root: __dirname,
-  output: {
-    dir: 'dist',
-  },
+  // test-only: used by tests to compute expected output path
+  output: { dir: 'dist' },
   build: {
+    outDir: 'dist',
     rollupOptions: {
       output: {
         assetFileNames: 'assets/[name][extname]',
         chunkFileNames: 'assets/[name].js',
         entryFileNames: 'assets/[name].js',
       },
     },
   },
   plugins: [stats()],
 });
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 0b65371 and 7692615.

⛔ Files ignored due to path filters (1)
  • test/package/vite/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (7)
  • test/package/vite/configs.test.ts (1 hunks)
  • test/package/vite/options.test.ts (1 hunks)
  • test/package/vite/package.json (1 hunks)
  • test/package/vite/vite-options.config.js (1 hunks)
  • test/package/vite/vite.config.js (1 hunks)
  • test/package/vite/vite.config.mjs (1 hunks)
  • test/package/vite/vite.config.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/package/vite/vite.config.ts (1)
test/package/vite/vite.config.js (1)
  • stats (3-3)
test/package/vite/vite-options.config.js (2)
test/package/vite/vite.config.js (1)
  • stats (3-3)
src/index.ts (1)
  • rollupStats (34-59)
test/package/vite/vite.config.mjs (1)
test/package/vite/vite.config.js (1)
  • stats (3-3)
🪛 GitHub Check: lint
test/package/vite/vite.config.js

[failure] 3-3:
A require() style import is forbidden


[failure] 2-2:
A require() style import is forbidden

🪛 GitHub Actions: ci
test/package/vite/vite.config.js

[error] 2-2: Command: eslint . | ESLint: A require() style import is forbidden. (@typescript-eslint/no-require-imports)

🔇 Additional comments (4)
test/package/vite/package.json (2)

6-11: LGTM: scripts cover CJS, ESM, and TS configs

The split build scripts look good and match the tests’ intent.


17-17: Verify local plugin resolution

"rollup-plugin-stats": "*" may pull from the registry instead of this repo in CI/local. Prefer a workspace/file link to ensure tests exercise the local code.

If using workspaces, consider:

  • "rollup-plugin-stats": "workspace:*"
    Otherwise:
  • "rollup-plugin-stats": "file:../../.." (adjust path to the plugin root)
test/package/vite/vite-options.config.js (1)

1-42: The above script will reveal whether the Vite test package is configured as ESM (via "type": "module" in its package.json). Once we confirm:

  • If there’s no "type": "module" in test/package/vite/package.json, then the current .js file must be treated as CommonJS by Node → the ESM import/export will indeed cause a runtime parse error.
  • If it does include "type": "module", then the ESM syntax is valid and no changes are needed (or you could choose the .mjs alternative).

Please review the output and let me know so we can finalize the recommendation.

test/package/vite/options.test.ts (1)

7-7: Exports and stats path in vite-options.config.js are correctly aligned

  • Default export (defineConfig) is defined at line 20.
  • Named exports relativeFileNameConfig (line 25) and absoluteFileNameConfig (line 34) are present as expected.
  • The relative stats file path ('../artifacts/stats-relative-path.json') matches the test’s expectation (line 29).

No further changes needed—imports, exports, and filename alignment in vite-options.config.js satisfy the test requirements.

@vio vio force-pushed the fix-commonjs-esm-output branch from 7692615 to 901e07b Compare August 27, 2025 21:58
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (4)
test/package/rollup/package.json (1)

7-7: Fix CJS config resolution: point to rollup.config.cjs

Use a .cjs filename to avoid ESM heuristics and keep the scenario strictly CommonJS.

Apply this diff:

-    "build-commonjs": "rollup -c rollup.config.js",
+    "build-commonjs": "rollup -c rollup.config.cjs",
test/package/vite/configs.test.ts (2)

18-22: Assert against combined output, not stderr only

Vite/Rollup info can land on stdout; combining reduces flakiness.

-      console.log(stdout);
-
-      expect(stderr).toMatch('Stats saved to');
-      expect(stderr).toMatch('built in');
+      console.log(stdout);
+      const output = stdout + stderr;
+      expect(output).toMatch('Stats saved to');
+      expect(output).toMatch('built in');

16-16: exec runs from repo root; pass cwd to the vite package (and bump buffer)

Without cwd, npm scripts won’t resolve to test/package/vite. Increase maxBuffer to avoid truncated logs.

-      const { stdout, stderr } = await exec(`cross-env NO_COLOR=true ${testCase.run}`);
+      const { stdout, stderr } = await exec(`cross-env NO_COLOR=true ${testCase.run}`, {
+        cwd: __dirname,
+        maxBuffer: 10 * 1024 * 1024,
+      });
test/package/vite/vite.config.js (1)

1-3: CI lint issue resolved

The directive spelling is fixed; require() exemptions now pass lint.

🧹 Nitpick comments (9)
test/package/rollup/package.json (2)

9-9: Prefer explicit plugin name for TS config loading

Make plugin resolution unambiguous across package managers.

Apply this diff:

-    "build-ts": "rollup --config rollup.config.ts --configPlugin typescript",
+    "build-ts": "rollup --config rollup.config.ts --configPlugin @rollup/plugin-typescript",

Please confirm this works in your CI matrix (npm/pnpm/yarn).


15-19: Ensure tests use the local plugin and harden the test package

Point the dependency to the workspace to test the current branch, not the latest published version. Also mark the test package private to prevent accidental publish.

Apply this diff:

-    "rollup-plugin-stats": "*",
+    "rollup-plugin-stats": "workspace:*",

And outside this hunk, add “private”: true at the top level:

 {
+  "private": true,
   "name": "rollup-plugin-stats-package-test",

If workspaces are not used, switch to a file: spec pointing to the repo root instead.

eslint.config.mjs (1)

16-16: Also ignore artifacts generated by tests

Extend ignores to cover artifacts/ directories created by the new test builds.

-    ignores: ['config/', '**/dist/'],
+    ignores: ['config/', '**/dist/**', '**/artifacts/**'],
test/package/vite/options.test.ts (1)

35-39: Clean up absolute tmp file to avoid cross-test leakage

Delete /tmp/custom-stats.json after the test to prevent residue between runs.

   test('should output stats JSON file with custom absolute filename', async () => {
     await vite(absoluteFileNameConfig);

     const actual = await fs.readFile('/tmp/custom-stats.json', 'utf8');
     const stats = JSON.parse(actual);
@@
     expect(stats['assets/index.js']).toMatchObject({
       fileName: 'assets/index.js',
     });
+    await fs.unlink('/tmp/custom-stats.json').catch(() => {});
   });
test/package/vite/configs.test.ts (2)

13-15: Fix suite/title: this file tests Vite, not Rollup

Rename for clarity.

-describe('rollup package - configs', () => {
+describe('vite package - configs', () => {
@@
-    test(`should build successfully with rollup ${testCase.type} config`, async () => {
+    test(`should build successfully with vite ${testCase.type} config`, async () => {

7-11: Minor naming nit: use “mjs” to match PR title/scripts

Purely cosmetic; aligns the label with the mjs variant.

 const testCases = [
   { type: 'commonjs', run: 'npm run build-commonjs' },
-  { type: 'esm', run: 'npm run build-esm' },
+  { type: 'mjs', run: 'npm run build-esm' },
   { type: 'ts', run: 'npm run build-ts' },
 ];
test/package/vite/vite.config.ts (1)

5-7: Top-level output is not a Vite option

Use build.outDir for the directory; keep Rollup output mapping under build.rollupOptions.output.

-export default defineConfig({
-  output: {
-    dir: 'dist',
-  },
-  build: {
+export default defineConfig({
+  build: {
+    outDir: 'dist',
     rollupOptions: {
       output: {
         assetFileNames: 'assets/[name][extname]',
         chunkFileNames: 'assets/[name].js',
         entryFileNames: 'assets/[name].js',
       },
     },
   },
   plugins: [stats()],
 });
test/package/vite/vite.config.js (2)

5-19: Scope the disable to the two require lines (optional)

Keep linting strict elsewhere.

-/* eslint-disable @typescript-eslint/no-require-imports */
-const { defineConfig } = require('vite');
-const stats = require('rollup-plugin-stats');
+/* eslint-disable-next-line @typescript-eslint/no-require-imports */
+const { defineConfig } = require('vite');
+/* eslint-disable-next-line @typescript-eslint/no-require-imports */
+const stats = require('rollup-plugin-stats');

6-17: Optional: set Vite outDir explicitly

Default is 'dist', but being explicit improves clarity.

 module.exports = defineConfig({
-  output: {
-    dir: 'dist',
-  },
   build: {
+    outDir: 'dist',
     rollupOptions: {
       output: {
         assetFileNames: 'assets/[name][extname]',
         chunkFileNames: 'assets/[name].js',
         entryFileNames: 'assets/[name].js',
       },
     },
   },
   plugins: [stats()],
 });
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 7692615 and 901e07b.

⛔ Files ignored due to path filters (2)
  • test/package/rollup/package-lock.json is excluded by !**/package-lock.json
  • test/package/vite/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (16)
  • .gitignore (1 hunks)
  • eslint.config.mjs (1 hunks)
  • test/package/rollup/configs.test.ts (1 hunks)
  • test/package/rollup/options.test.ts (1 hunks)
  • test/package/rollup/package.json (1 hunks)
  • test/package/rollup/rollup-options.config.mjs (1 hunks)
  • test/package/rollup/rollup.config.js (1 hunks)
  • test/package/rollup/rollup.config.mjs (1 hunks)
  • test/package/rollup/rollup.config.ts (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)
  • test/package/vite/vite-options.config.js (1 hunks)
  • test/package/vite/vite.config.js (1 hunks)
  • test/package/vite/vite.config.mjs (1 hunks)
  • test/package/vite/vite.config.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
  • test/package/vite/package.json
  • test/package/vite/vite-options.config.js
  • .gitignore
  • test/package/rollup/rollup-options.config.mjs
  • test/package/rollup/configs.test.ts
  • test/package/rollup/rollup.config.ts
  • test/package/vite/vite.config.mjs
  • test/package/rollup/rollup.config.js
  • test/package/rollup/rollup.config.mjs
  • test/package/rollup/options.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
test/package/vite/vite.config.ts (2)
test/package/vite/vite.config.js (1)
  • stats (3-3)
src/index.ts (1)
  • rollupStats (34-59)
⏰ 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 18 / ubuntu-latest
🔇 Additional comments (2)
test/package/vite/options.test.ts (1)

7-7: Import redirection LGTM

Switching to vite-options.config keeps tests focused on filename variants. No issues.

test/package/vite/configs.test.ts (1)

7-11: All three build scripts verified in test/package/vite/package.json

I’ve confirmed that build-commonjs, build-esm, and build-ts are defined under the "scripts" section in test/package/vite/package.json, so the test cases in configs.test.ts will run as intended—no updates required.

@vio vio force-pushed the fix-commonjs-esm-output branch from 901e07b to 3f49456 Compare August 27, 2025 22:25
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

♻️ Duplicate comments (1)
test/package/rollup/package.json (1)

7-7: Use .cjs for the CJS config to avoid ESM heuristics and match Node semantics

Rename the CJS config file to .cjs and update the script.

-    "build-commonjs": "rollup -c case-config.js",
+    "build-commonjs": "rollup -c case-config.cjs",
🧹 Nitpick comments (9)
test/package/vite/case-config.mjs (1)

9-17: Add build.outDir to set Vite’s output directory (keep top-level output for tests).

Vite reads build.outDir; the top-level output.dir is non-standard but harmless if tests read it. Add outDir: 'dist' for correctness without altering tests.

Apply this diff:

   build: {
+    outDir: 'dist',
     rollupOptions: {
       output: {
         assetFileNames: 'assets/[name][extname]',
         chunkFileNames: 'assets/[name].js',
         entryFileNames: 'assets/[name].js',
       },
     },
   },
test/package/vite/case-config.ts (1)

5-7: Use build.outDir instead of a top-level output in Vite TS config.

Top-level output isn’t a Vite field and may be flagged by TS types. Add build.outDir while keeping output if tests read it.

Apply this diff:

 export default defineConfig({
-  output: {
-    dir: 'dist',
-  },
   build: {
+    outDir: 'dist',
     rollupOptions: {
       output: {
         assetFileNames: 'assets/[name][extname]',
         chunkFileNames: 'assets/[name].js',
         entryFileNames: 'assets/[name].js',
       },
     },
   },

Also applies to: 8-16

test/package/vite/case-config.js (2)

1-1: ESLint disable is fine; consider a scoped override.

Since this is a CommonJS config, the inline disable is acceptable. Optionally move this to an ESLint override for config files to avoid file-level disables.


9-17: Explicitly set Vite’s build.outDir.

Add outDir: 'dist' for Vite to direct output; keep the existing top-level output if tests consume it.

Apply this diff:

   build: {
+    outDir: 'dist',
     rollupOptions: {
       output: {
         assetFileNames: 'assets/[name][extname]',
         chunkFileNames: 'assets/[name].js',
         entryFileNames: 'assets/[name].js',
       },
     },
   },
test/package/rollup/package.json (1)

15-19: Trim unused devDependencies; keep the fixture minimal

@rollup/plugin-typescript and cross-env aren’t used by these scripts/configs. Remove them. Also consider marking the package as private to avoid accidental publish.

   "devDependencies": {
-    "@rollup/plugin-typescript": "^12.0.0",
-    "cross-env": "^7.0.0",
     "rollup": "^4.0.0",
     "rollup-plugin-stats": "*",
     "typescript": "^5.0.0"
   }

Additionally (outside the changed range):

 {
   "name": "rollup-plugin-stats-package-test",
+  "private": true,
   "version": "1.0.0",
package.json (1)

54-56: Use npm --prefix instead of cd for cross-shell reliability.

cd ... && npm run ... works, but npm run --prefix <dir> <script> avoids shell quoting/Windows quirks and keeps cwd unchanged.

Apply:

-    "test:package:rolldown": "cd test/package/rolldown && npm run test",
-    "test:package:rollup": "cd test/package/rollup && npm run test",
-    "test:package:vite": "cd test/package/vite && npm run test",
+    "test:package:rolldown": "npm run --prefix test/package/rolldown test",
+    "test:package:rollup": "npm run --prefix test/package/rollup test",
+    "test:package:vite": "npm run --prefix test/package/vite test",
test/package/vite/package.json (3)

16-16: cross-env is unused as-is; either use it in scripts or drop it.

If you don’t need NO_COLOR (or any env var), remove cross-env. Otherwise, keep it and update the test script as suggested above.

Option A (remove):

   "devDependencies": {
     "vite": "^7.0.0",
-    "cross-env": "^7.0.0",
     "rollup-plugin-stats": "*"
   }

Option B (keep and use): see previous comment’s script diff.


6-11: Consider marking this test package as private to avoid accidental publish.

Not required, but common for fixture packages.

 {
   "name": "rollup-plugin-stats-package-test-vite",
+  "private": true,
   "version": "1.0.0",

6-11: Optional: remove unused “main” field in a pure test fixture.

Keeps the manifest minimal.

-  "main": "index.js",
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 901e07b and 3f49456.

⛔ Files ignored due to path filters (1)
  • test/package/rollup/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (17)
  • .gitignore (1 hunks)
  • package.json (1 hunks)
  • test/package/rolldown/options.test.js (1 hunks)
  • test/package/rolldown/package.json (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/case-options.js (3 hunks)
  • test/package/rollup/configs.test.ts (1 hunks)
  • test/package/rollup/options.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/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 (5)
  • .gitignore
  • test/package/rollup/configs.test.ts
  • test/package/rollup/options.test.ts
  • test/package/vite/options.test.ts
  • test/package/vite/configs.test.ts
🧰 Additional context used
🧬 Code graph analysis (6)
test/package/rollup/case-config.ts (2)
test/package/rollup/case-config.js (1)
  • stats (3-3)
src/index.ts (1)
  • rollupStats (34-59)
test/package/vite/case-config.ts (1)
test/package/vite/case-config.js (1)
  • stats (3-3)
test/package/vite/case-config.mjs (1)
test/package/vite/case-config.js (1)
  • stats (3-3)
test/package/rollup/case-config.js (1)
src/index.ts (1)
  • rollupStats (34-59)
test/package/rollup/case-config.mjs (2)
test/package/rollup/case-config.js (1)
  • stats (3-3)
src/index.ts (1)
  • rollupStats (34-59)
test/package/rollup/package.json (1)
src/index.ts (1)
  • rollupStats (34-59)
🔇 Additional comments (9)
test/package/rolldown/package.json (1)

7-7: Prefer non-watch CI mode: update test script to vitest run

Vitest is already declared in the workspace root package.json (devDependencies @3.2.4), so it’s available to all workspace packages.

  • File: test/package/rolldown/package.json, line 7

Apply this diff:

-    "test": "vitest"
+    "test": "vitest run"
test/package/rolldown/options.test.js (1)

9-9: Suite title change improves clarity.

Naming aligns with other packages’ option tests. No action needed.

test/package/vite/case-config.mjs (1)

18-19: Plugin wiring LGTM.

plugins: [stats()] is correct for Vite.

test/package/vite/case-config.ts (1)

17-18: Plugin wiring LGTM.

No issues with plugins: [stats()].

test/package/vite/case-config.js (1)

18-19: Plugin wiring LGTM.

CJS usage with module.exports = defineConfig({...}) is correct.

test/package/rollup/case-options.js (1)

5-5: Relative input path depends on CWD—please verify or update

I ran the provided grep and didn’t find any calls to process.chdir() or other cwd manipulation in the Rollup tests. That means we can’t automatically confirm the test harness is switching into test/package/rollup before invoking Rollup. Please manually verify one of the following:

• The test runner indeed sets process.cwd() to test/package/rollup (so 'src/index.js' resolves correctly),
• Or update the Rollup config to resolve the input path relative to the config file, for example:

import path from 'path';

input: path.resolve(__dirname, 'src/index.js'),

Affected locations:

  • test/package/rollup/case-options.js: line 5
  • test/package/rollup/case-options.js: line 13
  • test/package/rollup/case-options.js: line 24
  • test/package/rollup/case-options.js: line 36
test/package/rollup/case-config.ts (1)

1-10: LGTM: TS config mirrors other variants and uses stats with defaults

test/package/rollup/case-config.mjs (1)

1-10: LGTM: ESM config is minimal and consistent with TS/CJS variants

test/package/vite/package.json (1)

2-2: Rename looks good and avoids collisions with other package-test names.

@vio vio force-pushed the fix-commonjs-esm-output branch 6 times, most recently from 5c92f8f to df1c4dc Compare August 31, 2025 12:21
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (3)
test/package/vite/case-options.js (1)

22-29: Ensure artifacts directory is cleaned before tests
Tests write stats JSON into ../artifacts (e.g. stats-relative-path.json in test/package/**) but no cleanup step exists in any package.json or CI workflow—add a rimraf ../artifacts (or equivalent) pre-test to avoid stale files causing flakiness.

.github/workflows/ci.yml (2)

47-49: Use npm consistently; Yarn isn’t set up in this workflow.

The setup action provisions Node and npm, not Yarn. yarn run build will likely fail on fresh runners.

       - name: "Build"
-        run: yarn run build
+        run: npm run build

97-99: Windows runners: bash script won’t run without setting shell.

On windows-latest, default shell is PowerShell. Running a POSIX script without shell: bash will fail.

Apply to all three “Setup local npm registry” steps:

       - name: "Setup local npm registry"
-        run: ./scripts/setup-registry.sh
+        run: ./scripts/setup-registry.sh
+        shell: bash

Also applies to: 141-143, 191-193

🧹 Nitpick comments (5)
test/package/vite/case-options.js (1)

31-36: Make absolute path cross‑platform

Hardcoding '/tmp' will fail on Windows. Prefer os.tmpdir() + path.join for portability.

-    stats({
-      fileName: '/tmp/custom-stats.json',
-    }),
+    stats({
+      fileName: path.join(os.tmpdir(), 'custom-stats.json'),
+    }),

Add imports at the top of the file:

import os from 'node:os';
import path from 'node:path';
.github/workflows/ci.yml (2)

44-49: Make dependency installation explicit before build.

Unless your custom setup action installs deps by default, add an install step to avoid flaky builds.

       - name: "Setup node & npm"
         uses: "relative-ci/.github/actions/[email protected]"
 
+      - name: "Install"
+        run: npm ci
+
       - name: "Build"
-        run: yarn run build
+        run: npm run build

If the setup action already runs install, you can skip this.


55-55: Tighten artifact glob for nested files.

lib*/ may miss nested content on some shells. Prefer a recursive glob.

-          path: lib*/
+          path: lib/**/*
test/package/rolldown/package.json (1)

1-8: Mark the test package as private to prevent accidental publish.

 {
   "name": "rollup-plugin-stats-package-test",
+  "private": true,
   "version": "1.0.0",
   "description": "",
   "main": "index.js",
   "scripts": {
     "test": "vitest"
package.json (1)

29-29: Prefer the standard “types” field over “typings”.

Keeps alignment with TypeScript tooling; you already expose types via exports.

-  "typings": "./lib/types/index.d.ts",
+  "types": "./lib/types/index.d.ts",
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 9e170f1 and 4afeff0.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (27)
  • .github/workflows/ci.yml (1 hunks)
  • .gitignore (1 hunks)
  • eslint.config.mjs (1 hunks)
  • package.json (4 hunks)
  • rollup.config.mjs (1 hunks)
  • test/package/rolldown/options.test.js (1 hunks)
  • test/package/rolldown/package.json (1 hunks)
  • test/package/rollup/.gitignore (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/case-options.js (3 hunks)
  • test/package/rollup/configs.test.ts (1 hunks)
  • test/package/rollup/options.test.ts (1 hunks)
  • test/package/rollup/package.json (1 hunks)
  • test/package/vite/.gitignore (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.js (1 hunks)
  • test/package/vite/configs.test.ts (1 hunks)
  • test/package/vite/options.test.ts (2 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 due to trivial changes (3)
  • tsconfig.esm.json
  • test/package/rollup/.gitignore
  • test/package/vite/.gitignore
🚧 Files skipped from review as they are similar to previous changes (18)
  • test/package/rollup/case-config.ts
  • eslint.config.mjs
  • test/package/rollup/case-config.mjs
  • test/package/rollup/configs.test.ts
  • test/package/rollup/options.test.ts
  • test/package/vite/configs.test.ts
  • tsconfig.json
  • tsconfig.cjs.json
  • tsconfig.base.json
  • test/package/rollup/case-options.js
  • test/package/vite/options.test.ts
  • test/package/vite/case-config.js
  • test/package/rollup/package.json
  • test/package/vite/package.json
  • rollup.config.mjs
  • test/package/rollup/case-config.js
  • .gitignore
  • test/package/rolldown/options.test.js
🧰 Additional context used
🧬 Code graph analysis (3)
test/package/vite/case-options.js (1)
src/index.ts (1)
  • rollupStats (34-59)
test/package/vite/case-config.ts (1)
test/package/vite/case-config.js (1)
  • stats (3-3)
test/package/vite/case-config.mjs (1)
test/package/vite/case-config.js (1)
  • stats (3-3)
🔇 Additional comments (3)
test/package/vite/case-options.js (1)

5-6: Good switch to Vite’s build.outDir

Using build.outDir aligns with Vite semantics and the updated tests. Looks correct.

test/package/vite/case-config.mjs (1)

1-15: LGTM: minimal ESM config and plugin wiring look correct.

test/package/vite/case-config.ts (1)

1-15: LGTM: TS config mirrors the MJS variant correctly.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

♻️ Duplicate comments (1)
test/package/vite/package.json (1)

6-11: Test script may fail without local vitest; also not using cross-env despite being installed.

Declare vitest locally and wire cross-env to disable colors for stable output.

Apply:

   "scripts": {
     "build-cjs": "vite build -c case-config.js",
     "build-esm": "vite build -c case-config.mjs",
     "build-ts": "vite build -c case-config.ts",
-    "test": "vitest"
+    "test": "cross-env NO_COLOR=1 vitest"
   },

And add vitest:

   "devDependencies": {
     "cross-env": "^7.0.3",
     "rollup-plugin-stats": "*",
-    "vite": "^7.0.0"
+    "vite": "^7.0.0",
+    "vitest": "3.2.4"
   }
🧹 Nitpick comments (10)
test/package/vite/.gitignore (1)

1-2: Good ignores for build artifacts; consider adding node_modules (nit).

Not required, but adding node_modules is a common safeguard for test fixtures.

Apply:

 dist*
 package-lock.json
+node_modules/
test/package/vite/package.json (3)

15-17: Pin the plugin to the workspace or local file to test this PR, not the latest published.

Using "*" can pull a newer published version and make tests flaky.

Apply one of:

-    "rollup-plugin-stats": "*",
+    "rollup-plugin-stats": "workspace:*",

or (if not using workspaces):

-    "rollup-plugin-stats": "*",
+    "rollup-plugin-stats": "file:../../..",

3-5: Mark test package as private to avoid accidental publish.

Apply:

   "version": "1.0.0",
   "description": "",
+  "private": true,

5-5: Optional: drop “main” since this is a fixture not meant for consumption.

Keeps the manifest minimal.

Apply:

-  "main": "index.js",
tsconfig.json (4)

11-11: Drop redundant allowSyntheticDefaultImports when esModuleInterop=true.

TypeScript enables allowSyntheticDefaultImports when esModuleInterop is set.

     "esModuleInterop": true,
-    "allowSyntheticDefaultImports": true,

Also applies to: 16-16


15-15: Tighten allowJs unless you truly mix TS and JS in src.

If src is TS-only, setting allowJs=false avoids accidental JS creeping into builds and type-checking scope.

-    "allowJs": true,
+    "allowJs": false,

8-9: Align target/lib for consistency (optional).

Using lib ["es2020","es2022.error"] with target "es2020" is valid but a bit unusual. Consider either:

  • target "ES2022" and lib ["ES2022"], or
  • keep target "ES2020" and drop the ES2022-specific lib if Error.cause isn’t needed.

Example:

-    "target": "es2020",
-    "lib": ["es2020", "es2022.error"],
+    "target": "ES2022",
+    "lib": ["ES2022"],

18-27: Ensure declaration emit settings are properly configured

tsconfig.json enables "declaration": true and "declarationDir": "./lib/types" but also sets "noEmit": true, so no .d.ts files will ever be written. Neither tsconfig.cjs.json nor tsconfig.esm.json override noEmit or re-declare the declaration settings—they only set "outDir", inheriting the base’s noEmit: true. If you intend to ship types in lib/types, move and/or enable the declaration settings in your build configs:

Option A (base → type-check only, emit in build configs)
– In tsconfig.cjs.json and tsconfig.esm.json, under compilerOptions add:

{
  "emitDeclarationOnly": true,
  "declaration": true,
  "declarationDir": "./lib/types",
  "noEmit": false,
  "outDir": "./lib/cjs"    // or "./lib/esm"
}

Option B (emit from base)
– In tsconfig.json, remove or set "noEmit": false so that "declaration" and "declarationDir" take effect.

test/package/rollup/package.json (1)

1-21: Mark the test package as private to avoid accidental publish

Prevents accidental npm publish from CI or local runs.

Apply this diff:

 {
   "name": "rollup-plugin-stats-test-package-rollup",
+  "private": true,
   "version": "1.0.0",
test/package/rollup/configs.test.ts (1)

16-16: Optional: avoid cross-env by setting env in exec options

Reduces an external dependency in tests.

Apply this diff:

-      const { stdout, stderr } = await exec(`cross-env NO_COLOR=true ${testCase.run}`, { cwd });
+      const { stdout, stderr } = await exec(`${testCase.run}`, { cwd, env: { ...process.env, NO_COLOR: '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.

📥 Commits

Reviewing files that changed from the base of the PR and between 4afeff0 and 9f84c8c.

📒 Files selected for processing (16)
  • package.json (4 hunks)
  • test/package/rollup/.gitignore (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/.gitignore (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/configs.test.ts (1 hunks)
  • test/package/vite/package.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 (10)
  • tsconfig.cjs.json
  • tsconfig.esm.json
  • test/package/vite/case-config.mjs
  • test/package/vite/case-config.js
  • test/package/rollup/.gitignore
  • test/package/vite/configs.test.ts
  • test/package/rollup/case-config.js
  • test/package/rollup/case-config.ts
  • package.json
  • test/package/vite/case-config.ts
🧰 Additional context used
🧬 Code graph analysis (2)
test/package/rollup/case-config.mjs (2)
test/package/rollup/case-config.js (1)
  • stats (3-3)
src/index.ts (1)
  • rollupStats (34-59)
test/package/rollup/package.json (1)
src/index.ts (1)
  • rollupStats (34-59)
⏰ 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: tests
🔇 Additional comments (4)
test/package/vite/package.json (2)

6-9: Build scripts LGTM.

Covers CJS/ESM/TS via dedicated config files.


6-11: Tests reference updated scripts correctly
No occurrences of build-commonjs found in tests; they use npm run build-cjs, build-esm, and build-ts as expected.

tsconfig.json (2)

5-6: types and declarationDir look good; just ensure the path matches test expectations.

If tests assert types under lib/types, keep this stable across build configs.


28-29: Good include/exclude setup.

Limiting to "src" and excluding "lib" prevents editor/tsc from traversing build artifacts.

@vio vio force-pushed the fix-commonjs-esm-output branch from 9f84c8c to 60c58a8 Compare August 31, 2025 21:46
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
package.json (1)

62-66: Broken Husky hook: script name mismatch.

pre-commit runs “prettier” but only “format” exists; this blocks commits.

   "husky": {
     "hooks": {
-      "pre-commit": "npm run prettier && npm run lint"
+      "pre-commit": "npm run format && npm run lint"
     }
   },
♻️ Duplicate comments (2)
tsconfig.json (1)

12-13: Good call on NodeNext + nodenext.

This resolves package exports correctly in Node 18+.

package.json (1)

50-51: Make build-lib-packages cross‑platform.

POSIX redirection breaks on Windows shells. Use a Node one‑liner.

-    "build-lib-packages": "echo '{\"type\":\"commonjs\"}' > lib/cjs/package.json && echo '{\"type\":\"module\"}' > lib/esm/package.json",
+    "build-lib-packages": "node -e \"const fs=require('fs');fs.mkdirSync('lib/cjs',{recursive:true});fs.mkdirSync('lib/esm',{recursive:true});fs.writeFileSync('lib/cjs/package.json',JSON.stringify({type:'commonjs'}));fs.writeFileSync('lib/esm/package.json',JSON.stringify({type:'module'}));\"",
🧹 Nitpick comments (5)
test/package/rolldown/case-options.js (1)

41-43: Avoid hardcoding /tmp; use OS temp dir for portability.

Use node:os.tmpdir() to keep tests green on non-Unix environments.

-import statsPlugin({
-  fileName: '/tmp/custom-stats.json',
-}),
+import os from 'node:os';
+import path from 'node:path';
+// ...
+statsPlugin({
+  fileName: path.join(os.tmpdir(), 'custom-stats.json'),
+}),
tsconfig.json (3)

5-5: Root config mixes “typecheck-only” with emit settings.

noEmit: true makes declaration[Dir] moot here; keep emit flags only in build tsconfigs to avoid confusion.

-    "declarationDir": "./lib/types",
-    "declaration": true,
-    "noEmit": true
+    "noEmit": true

Also applies to: 19-19, 27-27


16-16: Consider dropping allowJs.

With "include": ["src"] and TS sources, allowJs is likely unnecessary; removing reduces surprises.

-    "allowJs": true,
+    // "allowJs": true,

7-11: Optional: add verbatimModuleSyntax for cleaner ESM/CJS interop.

Helps keep type-only imports untouched and avoids synthetic default surprises with NodeNext.

     "esModuleInterop": true,
+    "verbatimModuleSyntax": true,

Also applies to: 21-23

package.json (1)

27-29: Prefer “types” over “typings”.

“types” is the modern canonical field; keep exports.types too.

-  "typings": "./lib/types/index.d.ts",
+  "types": "./lib/types/index.d.ts",
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 9f84c8c and 60c58a8.

📒 Files selected for processing (11)
  • package.json (4 hunks)
  • test/package/rolldown/case-options.js (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/vite/case-config.js (1 hunks)
  • test/package/vite/case-config.mjs (1 hunks)
  • test/package/vite/case-config.ts (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 (8)
  • test/package/rollup/case-config.mjs
  • tsconfig.cjs.json
  • test/package/rollup/case-config.js
  • test/package/rollup/case-config.ts
  • test/package/vite/case-config.mjs
  • test/package/vite/case-config.js
  • tsconfig.esm.json
  • test/package/vite/case-config.ts
🧰 Additional context used
🧬 Code graph analysis (1)
test/package/rolldown/case-options.js (3)
test/package/rollup/case-config.js (1)
  • statsPlugin (3-3)
test/package/vite/case-config.js (1)
  • statsPlugin (3-3)
src/index.ts (1)
  • rollupStats (34-59)
⏰ 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). (9)
  • GitHub Check: vite v5 / node 18 / ubuntu-latest
  • GitHub Check: vite v6 / node 18 / ubuntu-latest
  • GitHub Check: vite v6 / node 22 / windows-latest
  • GitHub Check: vite v5 / node 20 / ubuntu-latest
  • GitHub Check: vite v5 / node 22 / windows-latest
  • GitHub Check: vite v6 / node 22 / ubuntu-latest
  • GitHub Check: vite v7 / node 20 / ubuntu-latest
  • GitHub Check: rollup v4 / node 18 / ubuntu-latest
  • GitHub Check: rollup v3 / node 22 / windows-latest
🔇 Additional comments (2)
test/package/rolldown/case-options.js (2)

29-31: LGTM: relative fileName intentionally resolves against output.dir.

Your relative path will resolve using generateBundle options.dir ('dist'), matching the plugin behavior in src/index.ts.


5-5: CWD assumption confirmed; no changes needed.
npm run test:package:rolldown includes cd test/package/rolldown, so input: 'src/index.js' resolves correctly.

@vio vio force-pushed the fix-commonjs-esm-output branch from 60c58a8 to f6aa6bb Compare August 31, 2025 21:59
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

♻️ Duplicate comments (1)
package.json (1)

49-51: Make build-lib-packages cross-platform.

Shell redirection breaks on Windows. Use a Node one-liner.

-    "build-lib-packages": "echo '{\"type\":\"commonjs\"}' > lib/cjs/package.json && echo '{\"type\":\"module\"}' > lib/esm/package.json",
+    "build-lib-packages": "node -e \"const fs=require('fs');fs.mkdirSync('lib/cjs',{recursive:true});fs.mkdirSync('lib/esm',{recursive:true});fs.writeFileSync('lib/cjs/package.json',JSON.stringify({type:'commonjs'}));fs.writeFileSync('lib/esm/package.json',JSON.stringify({type:'module'}));\"",
🧹 Nitpick comments (3)
test/package/rollup/case-config.ts (1)

6-8: Set an explicit output format to avoid default changes across Rollup versions.

Be explicit so the TS scenario isn't sensitive to Rollup defaults.

 export default defineConfig({
   input: 'src/index.js',
   output: {
+    format: 'esm',
     dir: 'dist-ts',
   },
   plugins: [statsPlugin({})],
 });
test/package/rollup/package.json (1)

9-9: Ensure the TS config plugin resolves reliably on all setups.

Some environments require the full package name in --configPlugin. Consider this tweak if CI flakes.

-    "build-ts": "rollup --config case-config.ts --configPlugin typescript",
+    "build-ts": "rollup --config case-config.ts --configPlugin @rollup/plugin-typescript",
package.json (1)

29-29: Prefer "types" over "typings" for TypeScript metadata.

Both work in TS, but "types" is the de-facto standard used by tooling.

-  "typings": "./lib/types/index.d.ts",
+  "types": "./lib/types/index.d.ts",
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 60c58a8 and 509aa8a.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (26)
  • .github/workflows/ci.yml (1 hunks)
  • .gitignore (1 hunks)
  • package.json (4 hunks)
  • rollup.config.mjs (1 hunks)
  • test/package/rolldown/case-options.js (1 hunks)
  • test/package/rolldown/options.test.js (1 hunks)
  • test/package/rolldown/package.json (1 hunks)
  • test/package/rollup/.gitignore (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/case-options.js (3 hunks)
  • test/package/rollup/configs.test.ts (1 hunks)
  • test/package/rollup/options.test.ts (1 hunks)
  • test/package/rollup/package.json (1 hunks)
  • test/package/vite/.gitignore (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.js (1 hunks)
  • test/package/vite/configs.test.ts (1 hunks)
  • test/package/vite/options.test.ts (2 hunks)
  • test/package/vite/package.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 (2)
  • test/package/rollup/.gitignore
  • test/package/vite/.gitignore
🚧 Files skipped from review as they are similar to previous changes (19)
  • .github/workflows/ci.yml
  • test/package/rolldown/case-options.js
  • test/package/vite/options.test.ts
  • test/package/vite/case-options.js
  • .gitignore
  • test/package/rollup/case-config.js
  • test/package/vite/case-config.mjs
  • test/package/vite/case-config.ts
  • tsconfig.cjs.json
  • test/package/rollup/configs.test.ts
  • test/package/vite/package.json
  • test/package/rollup/case-config.mjs
  • test/package/vite/case-config.js
  • test/package/vite/configs.test.ts
  • test/package/rollup/options.test.ts
  • test/package/rolldown/options.test.js
  • tsconfig.esm.json
  • test/package/rolldown/package.json
  • test/package/rollup/case-options.js
🧰 Additional context used
🧬 Code graph analysis (4)
test/package/rollup/case-config.ts (2)
test/package/rollup/case-config.js (1)
  • statsPlugin (3-3)
src/index.ts (1)
  • rollupStats (34-59)
package.json (1)
test/unit/extract.test.ts (1)
  • test (7-27)
rollup.config.mjs (2)
src/index.ts (1)
  • rollupStats (34-59)
src/extract.ts (1)
  • extractRollupStats (58-132)
test/package/rollup/package.json (1)
src/index.ts (1)
  • rollupStats (34-59)

Comment on lines +25 to +27
preserveModules: true,
preserveModulesRoot: CONTEXT,
interop: 'auto',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use a relative root for preserveModulesRoot (not an absolute path).

Passing an absolute path can leak full paths into output structure. Use "src".

-      preserveModulesRoot: CONTEXT,
+      preserveModulesRoot: 'src',

Also applies to: 50-52

🤖 Prompt for AI Agents
In rollup.config.mjs around lines 25-27 (and similarly 50-52),
preserveModulesRoot is set to an absolute CONTEXT value which can leak full
filesystem paths into the output; change it to a relative root string like "src"
(or another repo-relative directory) so Rollup emits module paths relative to
project source instead of absolute paths, update both occurrences to use the
relative "src" value, and ensure any related path usage elsewhere is adjusted to
treat CONTEXT as a workspace root rather than the preserveModulesRoot.

Comment on lines +31 to +33
nodeResolvePlugin({
extensions: ['.js', '.cjs', '.json'],
}),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Resolve .ts files in NodeResolve to avoid edge-case resolution failures.

Include TS extensions so intra-project TS imports are resolved consistently.

       nodeResolvePlugin({
-        extensions: ['.js', '.cjs', '.json'],
+        extensions: ['.js', '.cjs', '.ts', '.json'],
       }),
       nodeResolvePlugin({
-        extensions: ['.js', '.mjs', '.cjs', '.json'],
+        extensions: ['.js', '.mjs', '.cjs', '.ts', '.json'],
       }),

Also applies to: 56-58

🤖 Prompt for AI Agents
In rollup.config.mjs around lines 31-33 and again around 56-58, the
nodeResolvePlugin's extensions list omits TypeScript extensions causing
edge-case resolution failures; update both plugin configurations to include
TypeScript extensions (at minimum '.ts', and optionally '.tsx') alongside the
existing '.js', '.cjs', '.json' entries so intra-project TS imports resolve
consistently.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (1)
package.json (1)

64-64: Fix Husky hook: script name mismatch (“prettier” vs “format”)
Pre-commit currently calls a non-existent "prettier" script; CI/local commits will fail.

Two options; pick one:

  • Rename hook to call existing script:
-      "pre-commit": "npm run prettier && npm run lint"
+      "pre-commit": "npm run format && npm run lint"
  • Or add a "prettier" alias:
   "scripts": {
+    "prettier": "prettier --write .",
     "format": "prettier --write .",
♻️ Duplicate comments (1)
package.json (1)

49-51: Make post-build package.json generation cross‑platform and future‑proof
POSIX echo + single quotes isn’t portable on Windows; also create lib/cjs/type to guard against a future root "type": "module".

-    "build": "npm run clean && tsc && rollup -c && npm run build-esm-packages",
-    "build-esm-packages": "echo '{\"type\":\"module\"}' > lib/esm/package.json",
+    "build": "npm run clean && tsc && rollup -c && npm run build-esm-packages",
+    "build-esm-packages": "node -e \"const fs=require('fs');['lib/esm','lib/cjs'].forEach(d=>fs.mkdirSync(d,{recursive:true}));fs.writeFileSync('lib/esm/package.json',JSON.stringify({type:'module'}));fs.writeFileSync('lib/cjs/package.json',JSON.stringify({type:'commonjs'}));\"",
     "clean": "rimraf ./lib",
🧹 Nitpick comments (2)
package.json (2)

29-29: Prefer “types” over “typings”
Modern TS expects "types". Keep both only if you need legacy support.

Apply this minimal change:

-  "typings": "./lib/types/index.d.ts",
+  "types": "./lib/types/index.d.ts",

56-58: Optional: avoid subshell cd for package tests
Use npm’s -C/--prefix to reduce shell differences.

-    "test:package:rolldown": "cd test/package/rolldown && npm run test",
+    "test:package:rolldown": "npm -C test/package/rolldown run test",
-    "test:package:rollup": "cd test/package/rollup && npm run test",
+    "test:package:rollup": "npm -C test/package/rollup run test",
-    "test:package:vite": "cd test/package/vite && npm run test",
+    "test:package:vite": "npm -C test/package/vite run test",
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 509aa8a and d3de676.

📒 Files selected for processing (1)
  • package.json (4 hunks)
⏰ 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 18 / ubuntu-latest
🔇 Additional comments (6)
package.json (6)

4-4: Confirm prerelease intent and release flow

Version bumped to 1.5.1-beta.1. Verify release tooling (release-it) and npm dist-tags are configured for prereleases.


27-28: LGTM: entry points migrated to lib/
Main/module paths look correct for CJS/ESM split.


31-31: LGTM: package files include lib/
This aligns with the new output location.


35-37: LGTM: conditional exports for root entry
The import/require/types mapping is consistent.


70-73: LGTM: devDeps align with Rollup 4/TS 5
The added/updated Rollup plugins and rimraf look compatible.

Also applies to: 83-83


40-42: Confirm extract artifacts are built and included
I couldn’t locate the lib/esm, lib/cjs, or lib/types directories containing the expected files. After running your build process, ensure these dirs include index.js, extract.js, and their .d.ts counterparts before publishing.

@vio vio force-pushed the fix-commonjs-esm-output branch from d3de676 to d367c60 Compare September 2, 2025 20:27
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
rollup.config.mjs (2)

13-14: Remove misuse of Rollup “context” and avoid absolute preserveModulesRoot; drop CONTEXT entirely.

  • context sets the top-level this in modules, not a filesystem path. Feeding an absolute path here is misleading and can change runtime semantics.
  • preserveModulesRoot should be a repo-relative root like 'src', not an absolute path, to avoid path leakage and improve portability.
  • With these changes, CONTEXT becomes unnecessary.

Apply:

-const CONTEXT = path.join(import.meta.dirname, 'src');
+// CONTEXT not needed; use a relative root in preserveModulesRoot

@@
-    context: CONTEXT,
+    // use default module context

@@
-      preserveModulesRoot: CONTEXT,
+      preserveModulesRoot: 'src',
@@
-    context: CONTEXT,
+    // use default module context

@@
-      preserveModulesRoot: CONTEXT,
+      preserveModulesRoot: 'src',

Also applies to: 18-18, 26-27, 43-43, 51-52


31-33: Include TypeScript extensions in node-resolve to prevent edge-case resolution failures.

Add .ts (and optionally .tsx) so TS intra-repo imports resolve consistently in both configs.

       nodeResolvePlugin({
-        extensions: ['.js', '.cjs', '.json'],
+        extensions: ['.js', '.cjs', '.ts', '.json'],
       }),
@@
       nodeResolvePlugin({
-        extensions: ['.js', '.mjs', '.cjs', '.json'],
+        extensions: ['.js', '.mjs', '.cjs', '.ts', '.json'],
       }),

Also applies to: 56-58

🧹 Nitpick comments (2)
rollup.config.mjs (2)

52-52: Drop interop from the ESM output to avoid confusion.

output.interop is relevant for CJS output; it’s a no-op for ESM.

-      interop: 'auto',
+      // interop is relevant for CJS only

29-29: Make external more robust by externalizing bare imports, not only paths containing node_modules.

The regex misses bare IDs (e.g., dependencies resolved by Rollup). Prefer a function that treats non-relative, non-absolute imports as external.

-    external: /node_modules/,
+    external: (id) => !id.startsWith('.') && !path.isAbsolute(id),
-    external: /node_modules/,
+    external: (id) => !id.startsWith('.') && !path.isAbsolute(id),

Also applies to: 54-54

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between d3de676 and 5ddef54.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (2)
  • package.json (4 hunks)
  • rollup.config.mjs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • package.json
🧰 Additional context used
🧬 Code graph analysis (1)
rollup.config.mjs (1)
src/index.ts (1)
  • rollupStats (34-59)

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
package.json (1)

49-52: Make build-esm-packages cross‑platform (Windows-safe)

echo with single quotes breaks on Windows. Use a Node one‑liner.

-    "build-esm-packages": "echo '{\"type\":\"module\"}' > lib/esm/package.json",
+    "build-esm-packages": "node -e \"const fs=require('fs');fs.mkdirSync('lib/esm',{recursive:true});fs.writeFileSync('lib/esm/package.json',JSON.stringify({type:'module'}));\"",
🧹 Nitpick comments (5)
tsconfig.json (2)

7-11: Align lib with target and add verbatimModuleSyntax

With target set to esnext, consider updating lib and enabling verbatimModuleSyntax for cleaner ESM interop.

-    "lib": ["es2020", "es2022.error"],
+    "lib": ["ES2022", "ES2022.Error"],
     "module": "nodenext",
     "moduleResolution": "NodeNext",
     "resolveJsonModule": true,
+    "verbatimModuleSyntax": true,

3-6: Narrow rootDir to src to avoid accidental includes

rootDir "." can pull in config/test TS by accident. Prefer keeping sources under src.

-    "rootDir": ".",
+    "rootDir": "./src",
tsconfig.esm.json (1)

3-9: Prefer verbatimModuleSyntax with bundler resolution

Prevents TS from transforming imports/exports and aligns with Rollup/Vite expectations.

   "compilerOptions": {
     "module": "ESNext",
     "moduleResolution": "bundler",
+    "verbatimModuleSyntax": true,
tsconfig.base.json (2)

13-24: noEmit: true in base requires explicit overrides in build configs

Since this base is shared, ensure tsconfig.cjs.json also sets noEmit: false (or emitDeclarationOnly) like the ESM config so both targets produce .d.ts in lib/types.

I can draft a matching tsconfig.cjs.json diff if you want.


4-11: Minor: unify lib capitalization and consider verbatimModuleSyntax

Small consistency nit: use ES2022 and ES2022.Error casing; also consider adding verbatimModuleSyntax in base if consumers use NodeNext/Bundler widely.

-    "lib": ["es2020", "es2022.error"],
+    "lib": ["ES2020", "ES2022.Error"],
+    "verbatimModuleSyntax": 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 5ddef54 and b65e90e.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (5)
  • package.json (4 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 (1)
  • tsconfig.cjs.json
🧰 Additional context used
🧬 Code graph analysis (3)
tsconfig.base.json (1)
src/index.ts (1)
  • rollupStats (34-59)
tsconfig.json (1)
test/unit/extract.test.ts (1)
  • test (7-27)
package.json (1)
test/unit/extract.test.ts (1)
  • test (7-27)
🔇 Additional comments (4)
tsconfig.json (2)

24-24: Excluding tests from type-checking—confirm intent

exclude includes "test", so test/unit/*.ts won’t be checked by npm run type-check. If unintended, drop "test" from exclude or add a dedicated tsconfig for tests.


14-16: Per-target tsconfigs confirmed in Rollup config
Root tsconfig’s noEmit only affects editor/type-checking; the Rollup plugin explicitly uses tsconfig.cjs.json and tsconfig.esm.json, so declaration files will be emitted as expected.

package.json (2)

49-51: Type emission path depends on tsconfig overrides

npm run type-check runs tsc (no emit), so declaration files must come from Rollup’s TS plugin. Ensure the per-target tsconfigs explicitly override noEmit to allow declarations to be written to lib/types.


27-43: Build config correctly emits all exported artifacts

The Rollup setup with tsconfig.cjs.json and tsconfig.esm.json produces
• CJS files under lib/cjs/{index,extract}.js
• ESM files under lib/esm/{index,extract}.js
• Type declarations under lib/types/{index,extract}.d.ts

No further changes needed.

Comment on lines 3 to 7
"compilerOptions": {
"module": "ESNext",
"moduleResolution": "bundler",
"outDir": "./lib/esm",
"declarationDir": "./lib/types",
"declaration": true
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enable declaration emission here (base sets noEmit: true)

Base tsconfig has noEmit: true; without overriding, declarations won’t be written. Turn on emitting (or emitDeclarationOnly) and consider declarationMap for DX.

   "compilerOptions": {
     "module": "ESNext",
     "moduleResolution": "bundler",
     "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.

Suggested change
"compilerOptions": {
"module": "ESNext",
"moduleResolution": "bundler",
"outDir": "./lib/esm",
"declarationDir": "./lib/types",
"declaration": true
}
"compilerOptions": {
"module": "ESNext",
"moduleResolution": "bundler",
"outDir": "./lib/esm",
"declarationDir": "./lib/types",
"declaration": true,
"emitDeclarationOnly": true,
"noEmit": false,
"declarationMap": true
}
🤖 Prompt for AI Agents
In tsconfig.esm.json around lines 3 to 9, the base tsconfig sets "noEmit": true
so declarations configured here won't be emitted; update compilerOptions to
enable emission by adding either "emitDeclarationOnly": true (preferred for
declaration-only builds) or set "noEmit": false, and optionally add
"declarationMap": true to generate .d.ts.map files for better DX; ensure these
options are merged with existing keys (keep "declaration": true) so .d.ts files
are actually written to ./lib/types.

@vio vio force-pushed the fix-commonjs-esm-output branch from b65e90e to 1361912 Compare September 27, 2025 10:06
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (1)
test/package/vite/case-options.js (1)

1-38: Reintroduce the dynamicOptions export for parity with tests.

options.test.ts in the same package still imports dynamicOptions from this module. Since that export no longer exists, the Vitest run will throw dynamicOptions is not defined, breaking the suite. Please restore (or reimplement) the dynamicOptions variant alongside the default/relative/absolute configs.

🧹 Nitpick comments (4)
package.json (2)

48-62: Ensure builds run before publish

Add a prepublishOnly/prepack to avoid publishing without lib outputs.

Apply:

   "scripts": {
     "build": "npm run clean && npm run type-check && rollup -c && npm run build-esm-packages",
     "build-esm-packages": "node -e \"const fs=require('fs');fs.mkdirSync('lib/esm',{recursive:true});fs.writeFileSync('lib/esm/package.json',JSON.stringify({type:'module'}));\"",
     "type-check": "tsc",
     "clean": "rimraf ./lib",
+    "prepublishOnly": "npm run build",
+    "prepack": "npm run build",

29-29: Prefer “types” over “typings”

TypeScript recognizes both, but “types” is the modern convention.

Apply:

-  "typings": "./lib/types/index.d.ts",
+  "types": "./lib/types/index.d.ts",
tsconfig.lib-esm.json (1)

3-7: Explicitly override noEmit for declaration output

Base tsconfig.lib.json sets "noEmit": true. Add "noEmit": false here so declaration files are reliably emitted to lib/types during the ESM build (avoid relying on plugin defaults).

Based on learnings

   "compilerOptions": {
     "outDir": "./lib/esm",
     "declarationDir": "./lib/types",
-    "declaration": true
+    "declaration": true,
+    "noEmit": false
   }
tsconfig.lib-cjs.json (1)

3-5: Clarify emit behavior in CJS build

To avoid confusion with the base "noEmit": true, set it to false here too (even if only ESM build emits types).

   "compilerOptions": {
-    "outDir": "./lib/cjs"
+    "outDir": "./lib/cjs",
+    "noEmit": false
   }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b65e90e and 1361912.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (29)
  • .github/workflows/ci.yml (1 hunks)
  • .gitignore (1 hunks)
  • eslint.config.mjs (1 hunks)
  • package.json (3 hunks)
  • rollup.config.mjs (1 hunks)
  • test/package/rolldown/case-options.js (1 hunks)
  • test/package/rolldown/options.test.js (1 hunks)
  • test/package/rolldown/package.json (1 hunks)
  • test/package/rollup/.gitignore (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/case-options.js (3 hunks)
  • test/package/rollup/configs.test.ts (1 hunks)
  • test/package/rollup/options.test.ts (1 hunks)
  • test/package/rollup/package.json (1 hunks)
  • test/package/vite/.gitignore (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.js (1 hunks)
  • test/package/vite/configs.test.ts (1 hunks)
  • test/package/vite/options.test.ts (2 hunks)
  • test/package/vite/package.json (1 hunks)
  • test/unit/extract.test.ts (1 hunks)
  • tsconfig.json (1 hunks)
  • tsconfig.lib-cjs.json (1 hunks)
  • tsconfig.lib-esm.json (1 hunks)
  • tsconfig.lib.json (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • .gitignore
🚧 Files skipped from review as they are similar to previous changes (12)
  • test/package/rollup/.gitignore
  • test/package/vite/.gitignore
  • test/package/vite/options.test.ts
  • test/package/vite/case-config.ts
  • test/package/vite/package.json
  • test/package/vite/case-config.mjs
  • test/package/rollup/case-options.js
  • test/package/vite/configs.test.ts
  • test/package/rolldown/case-options.js
  • test/package/rollup/case-config.ts
  • eslint.config.mjs
  • test/package/rollup/configs.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
test/package/rollup/case-config.mjs (2)
test/package/rollup/case-config.js (1)
  • statsPlugin (3-3)
test/package/vite/case-config.js (1)
  • statsPlugin (3-3)
test/package/vite/case-config.js (1)
test/package/rollup/case-config.js (2)
  • require (2-2)
  • statsPlugin (3-3)
test/package/rollup/case-config.js (1)
test/package/vite/case-config.js (2)
  • require (2-2)
  • statsPlugin (3-3)
⏰ 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: rolldown v1 / node 22 / windows-latest
🔇 Additional comments (11)
test/unit/extract.test.ts (2)

6-9: LGTM: typing rollup fixtures via OutputBundle is appropriate

The double cast to OutputBundle for test fixtures is reasonable here.


12-29: LGTM: refactor to reuse typed fixtures

Switching to deepFreeze(fixtures) across tests keeps intent clear and avoids repetition.

package.json (3)

27-43: Exports and entrypoints look consistent with lib/{cjs,esm}/ + types

Mapping import/require/types to lib paths aligns with the new build layout.


49-52: Make build-esm-packages cross‑platform

POSIX echo + single quotes fail on Windows shells. Prefer a Node one‑liner.

Apply:

-    "build": "npm run clean && npm run type-check && rollup -c && npm run build-esm-packages",
-    "build-esm-packages": "echo '{\"type\":\"module\"}' > lib/esm/package.json",
+    "build": "npm run clean && npm run type-check && rollup -c && npm run build-esm-packages",
+    "build-esm-packages": "node -e \"const fs=require('fs');fs.mkdirSync('lib/esm',{recursive:true});fs.writeFileSync('lib/esm/package.json',JSON.stringify({type:'module'}));\"",

88-88: No update required for Vitest version
[email protected] is published on npm and is the latest stable release.

tsconfig.lib.json (1)

25-29: noEmit in base config is fine for type‑check only

Keeping "noEmit": true here is OK since build configs override it. Ensure only one TS config emits declarations to avoid duplicate writes.

If both Rollup configs run in parallel, confirm only the ESM config has "declaration": true to prevent concurrent writes to ./lib/types.

rollup.config.mjs (3)

18-18: Remove the misuse of Rollup’s context option

context controls the top-level this value for modules; pointing it at your source directory is misleading and can trip code that assumes the default. Please drop it.

Also applies to: 43-43


25-27: Use a relative preserveModulesRoot

Feeding preserveModulesRoot an absolute path bakes machine-specific directories into the build graph. Use a stable project-relative root instead (e.g. 'src').

Also applies to: 50-52


31-33: Include TypeScript extensions in NodeResolve

Without .ts the resolver misses TS source imports, leaving TypeScript plugin to paper over it. Add .ts (and optionally .tsx) to both resolver instances for consistent resolution.

Also applies to: 56-58

test/package/rollup/case-config.js (1)

2-11: Drop the ESM-only Rollup import from this CJS config

require('rollup') throws ERR_REQUIRE_ESM, so this config crashes before the test even runs. Export a plain object instead of piping through defineConfig.

-const { defineConfig } = require('rollup');
 const statsPlugin = require('rollup-plugin-stats');

-module.exports = defineConfig({
+module.exports = {
   input: 'src/index.js',
   output: {
     dir: 'dist-cjs',
   },
   plugins: [statsPlugin({})],
-});
+};
test/package/rollup/case-config.mjs (1)

4-9: Add the required output.format

Rollup refuses to generate bundles without output.format, so this test config currently errors out. Set it to es.

 export default defineConfig({
   input: 'src/index.js',
   output: {
     dir: 'dist-mjs',
+    format: 'es',
   },
   plugins: [statsPlugin({})],
 });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants