fix(vite-config): use Node16 module for CJS declaration#352
Conversation
🦋 Changeset detectedLatest commit: 5325427 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded
⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpdates TypeScript compiler setting for the CommonJS dts build to Node16 semantics and adds a changeset file documenting a patch bump and upgrade note from Node10 to Node16. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
|
View your CI Pipeline Execution ↗ for commit 5325427
☁️ Nx Cloud last updated this comment at |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.changeset/empty-items-bet.md (1)
5-5: Polish the changeset sentence for clearer release notes.Consider using canonical capitalization/wording (e.g.,
CJS,Node10,Node16) to make the changelog easier to scan.Suggested wording
-fix: upgrade from cjs node10 to node16 +fix: upgrade CJS declaration emit from Node10 to Node16🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/empty-items-bet.md at line 5, Update the changeset title line "fix: upgrade from cjs node10 to node16" to use canonical capitalization/wording for clarity in release notes (e.g., change to "fix: upgrade from CJS / Node 10 to Node 16" or "fix: upgrade from CJS and Node10 to Node16"); edit the existing changeset sentence (the header string in the diff) to one of these normalized forms so the changelog is easier to scan.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.changeset/empty-items-bet.md:
- Line 5: Update the changeset title line "fix: upgrade from cjs node10 to
node16" to use canonical capitalization/wording for clarity in release notes
(e.g., change to "fix: upgrade from CJS / Node 10 to Node 16" or "fix: upgrade
from CJS and Node10 to Node16"); edit the existing changeset sentence (the
header string in the diff) to one of these normalized forms so the changelog is
easier to scan.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de9ca633-d406-4470-817e-2a432c08c194
📒 Files selected for processing (2)
.changeset/empty-items-bet.mdpackages/vite-config/src/index.ts
birkskyum
changed the title
lachlancollins
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/vite-config/src/index.ts (2)
59-62: Consider using named constants for TypeScript module kinds.Both
dts()configurations use magic numbers (99,100) for TypeScript'sModuleKindenum. While the inline comments help, extracting these to named constants would improve maintainability and make future updates clearer.♻️ Proposed refactor using named constants
+// TypeScript ModuleKind enum values +const TS_MODULE_ESNEXT = 99 +const TS_MODULE_NODE16 = 100 + export const tanstackViteConfig = (options: Options): UserConfig => {Then update the usages:
compilerOptions: { - module: 99, // ESNext + module: TS_MODULE_ESNEXT, declarationMap: false, },compilerOptions: { - module: 100, // Node16 + module: TS_MODULE_NODE16, declarationMap: false, },Also applies to: 85-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vite-config/src/index.ts` around lines 59 - 62, The code uses magic numbers for TypeScript ModuleKind in the dts() configurations (compilerOptions.module: 99 and 100); create named constants (e.g., const MODULE_KIND_ESNEXT = /* ModuleKind.ESNext value */ and const MODULE_KIND_ES2020 = /* ModuleKind.ES2020 value */ or use the actual enum import) and replace the numeric literals in the dts() configuration blocks (references: compilerOptions.module in the two dts() usages around the current module values) with those constants to improve readability and maintainability.
86-86: Change correctly enables Node16 module resolution for CJS declaration emit.Updating
module: 1tomodule: 100properly switches to Node16 resolution, which understandsexportsmaps inpackage.jsonand resolves TS2742 and TS2307 errors.Consider using named constants or TypeScript's
ModuleKindenum instead of magic numbers (99, 100) for better maintainability and clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vite-config/src/index.ts` at line 86, The TS config currently uses the magic number "module: 100" (in the TS options object where the module field is set) to enable Node16 module resolution; replace the numeric literal with a named constant from TypeScript (e.g. ModuleKind.Node16 or ts.ModuleKind.Node16) by importing ModuleKind/ts and assigning module: ModuleKind.Node16 (and do the same for any other magic values like 99), and update the inline comment accordingly so the intent is clear and maintainable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/vite-config/src/index.ts`:
- Around line 59-62: The code uses magic numbers for TypeScript ModuleKind in
the dts() configurations (compilerOptions.module: 99 and 100); create named
constants (e.g., const MODULE_KIND_ESNEXT = /* ModuleKind.ESNext value */ and
const MODULE_KIND_ES2020 = /* ModuleKind.ES2020 value */ or use the actual enum
import) and replace the numeric literals in the dts() configuration blocks
(references: compilerOptions.module in the two dts() usages around the current
module values) with those constants to improve readability and maintainability.
- Line 86: The TS config currently uses the magic number "module: 100" (in the
TS options object where the module field is set) to enable Node16 module
resolution; replace the numeric literal with a named constant from TypeScript
(e.g. ModuleKind.Node16 or ts.ModuleKind.Node16) by importing ModuleKind/ts and
assigning module: ModuleKind.Node16 (and do the same for any other magic values
like 99), and update the inline comment accordingly so the intent is clear and
maintainable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fab36a6-bc33-4763-a18b-6926811cf352
📒 Files selected for processing (1)
packages/vite-config/src/index.ts
lachlancollins
left a comment
lachlancollins
left a comment
There was a problem hiding this comment.
After a bit of testing, the declaration output between CJS and ESM is actually identical, regardless of what module is set to. The only difference is whether it can resolve other files and therefore successfully compile. It seems safe to change this to NodeNext for both the ESM and CJS declaration output. Unfortunately the duplication is still necessary for dual packaging!
|
🎉 This PR has been released! Thank you for your contribution! |
2 participants
Change the CJS
dts()pass frommodule: 1(Node10) tomodule: 100(Node16)Problem
module: 1(CommonJS) forces TypeScript to usenode10module resolution when generating CJS declarations (.d.cts). This causes two classes of build failures in downstream monorepos (Router - migrating from /config to the /vite-config package):TS2742 ("inferred type cannot be named") -
node10resolution doesn't understand package.jsonexportsmaps, so it resolves types through physical pnpm store paths (e.g.../../router-generator/node_modules/@tanstack/virtual-file-routes/dist/cjs/types.cjs) which aren't portable.TS2307 ("Cannot find module") - Packages that only define an
importcondition in theirexports(norequire) are invisible tonode10resolution, causing the CJS declaration pass to fail even though the types exist.Fix
module: 100(Node16) is CJS-aware and understandsexportsmaps, so TypeScript resolves types through package names rather than filesystem paths. ThebeforeWriteFilehook continues to handle.d.ts→.d.ctsrenaming and import extension rewriting as before.Summary by CodeRabbit