feat: sync internal with latest Node.js upstream - #49
Conversation
Sync the forked ESM resolver internals against current Node.js `main`, fix correctness bugs found during the diff, and add regression tests. - errors.ts: full sync (class-based NodeError machinery, determineSpecificType, splice fix, drop removed ERR_NETWORK_IMPORT_DISALLOWED), and fix createError return type so error instances carry .code/.url (removes @ts-expect-error) - get-format.ts: full sync of structure + anchor the data: MIME regex - resolve.ts: allow `#/`-prefixed subpath imports; fix conditional-exports null/undefined fall-through in resolvePackageTarget - builtins.ts: verified in sync with Node v24.x (node:-only modules excluded) - tests: add regression coverage for all changed surfaces Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 22 minutes and 40 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughUpdates error class construction from closure-based to class-based ChangesError System, Format Detection, and Resolution Logic
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #49 +/- ##
===========================================
+ Coverage 50.66% 64.62% +13.95%
===========================================
Files 6 6
Lines 825 896 +71
Branches 307 331 +24
===========================================
+ Hits 418 579 +161
+ Misses 306 257 -49
+ Partials 101 60 -41 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
pi0
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/internal-errors.test.ts (1)
81-87: ⚡ Quick winAdd coverage for
ERR_UNSUPPORTED_DIR_IMPORT.urlassignment.This suite verifies message formatting, but not the new
.urlside-effect path for the 3rd constructor argument.Suggested test addition
it("formats ERR_UNSUPPORTED_DIR_IMPORT message", () => { const err = new ERR_UNSUPPORTED_DIR_IMPORT("/dir", "/base"); expect(err.message).toBe( "Directory import '/dir' is not supported resolving ES modules imported from /base", ); }); + + it("sets url when exact url is provided for ERR_UNSUPPORTED_DIR_IMPORT", () => { + const err = new ERR_UNSUPPORTED_DIR_IMPORT( + "/dir", + "/base", + "file:///dir/index.js", + ); + expect(err.url).toBe("file:///dir/index.js"); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/internal-errors.test.ts` around lines 81 - 87, The current test for ERR_UNSUPPORTED_DIR_IMPORT only verifies the message formatting but does not test the `.url` property assignment. Add a new test case in the same suite that instantiates ERR_UNSUPPORTED_DIR_IMPORT with a third constructor argument and verifies that the resulting error object has the `.url` property correctly set to that third argument value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/internal/errors.ts`:
- Line 247: Replace the self-comparison pattern `value !== value` with
`Number.isNaN(value)` at line 247 in the errors.ts file. The self-comparison is
flagged by Biome's noSelfCompare rule, and Number.isNaN() is the standard and
recommended approach for detecting NaN values.
- Around line 266-269: The condition checking `if (value.constructor && "name"
in value.constructor)` can throw a TypeError when value.constructor is a
primitive value like a string, because the `in` operator requires an object on
its right-hand side. Add a type guard to verify that value.constructor is
actually an object before using the `in` operator on it, for example by checking
`typeof value.constructor === "object"` in addition to the existing truthiness
check, to prevent TypeError from being thrown during error message formatting.
In `@test/internal-get-format.test.ts`:
- Around line 88-90: The test uses a Unix-style file URL (file:///tmp/.foobar)
which fails on Windows because getFormat() calls getPackageScopeConfig()
internally when extname() returns empty string for dotfiles, and fileURLToPath()
throws an error on Windows with non-Windows paths. Replace the hardcoded /tmp/
path with a platform-appropriate file URL using conditional logic based on the
platform or by using import.meta.url as a base to construct a valid file URL
that works on both Windows and Unix systems. Apply the same fix to other tests
in this file that use /tmp paths and call getPackageScopeConfig.
---
Nitpick comments:
In `@test/internal-errors.test.ts`:
- Around line 81-87: The current test for ERR_UNSUPPORTED_DIR_IMPORT only
verifies the message formatting but does not test the `.url` property
assignment. Add a new test case in the same suite that instantiates
ERR_UNSUPPORTED_DIR_IMPORT with a third constructor argument and verifies that
the resulting error object has the `.url` property correctly set to that third
argument value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fa983a2-b4c8-401f-b89e-1d01df67f0d4
📒 Files selected for processing (15)
src/internal/builtins.tssrc/internal/errors.tssrc/internal/get-format.tssrc/internal/resolve.tstest/fixture/exports-pkg/default.jstest/fixture/exports-pkg/index.jstest/fixture/exports-pkg/package.jsontest/fixture/exports-pkg/worker.jstest/fixture/imports-pkg/index.jstest/fixture/imports-pkg/internal.jstest/fixture/imports-pkg/package.jsontest/fixture/imports-pkg/src/util.jstest/internal-errors.test.tstest/internal-get-format.test.tstest/resolve-imports-exports.test.ts
- use Number.isNaN(value) instead of self-comparison for NaN check - guard constructor.name access to avoid TypeError on primitive constructor (aligns determineSpecificType with upstream Node) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
This PR carefully diffs each forked file against current Node.js
main, syncs the lagging logic, fixes correctness bugs surfaced by the comparison, and adds regression tests for every changed surface.Changes & end-user impact
resolve.ts#/-prefixed subpath imports (Node dropped this restriction in upstream commite8c9c43)resolveModuleURL("#/foo.js", …)now resolves against a package'simportsmap instead of throwingERR_INVALID_MODULE_SPECIFIER.resolve.tsnullvsundefinedreturn inresolvePackageTargetfor conditional targetsexportswhere a nested condition object matches nothing now correctly falls through todefault(and sibling conditions) instead of wrongly throwingERR_PACKAGE_PATH_NOT_EXPORTED. Fewer false "subpath not exported" failures.get-format.tsdata:MIME-detection regex (/^…$/)text/javascriptxare no longer mis-detected as JavaScript modules — correct format classification fordata:URLs.errors.tsERR_INVALID_ARG_TYPEsplicefix (was a no-opslice)errors.tsNodeErrormachinery +determineSpecificTyperewritekIsNodeError, precise "Received …" details for bigint/-0/NaN/Infinity/symbol/etc.).ERR_MODULE_NOT_FOUND/ERR_UNSUPPORTED_DIR_IMPORTnow seterror.urlforimport.meta.resolveconsumers.errors.tscreateErrorso error instances carry.code/.url@ts-expect-errorworkarounds inresolve.ts. No runtime change.errors.tsERR_NETWORK_IMPORT_DISALLOWEDbuiltins.tsnode:-prefix-only modules (sqlite,test,sea) remain correctly excluded so baresqlite/testdon't falsely resolve as builtins.Most changes are behaviour-preserving syncs; the user-visible behaviour changes are the three resolver/format bug fixes in the first rows (all in the more-correct, more-permissive direction).
Intentionally left unsynced
legacyMainResolve/packageResolveC++-binding refactor + experimental package-map feature (the JS implementation remains correct for standard resolution; tracked by the existing headerTODO).detectModuleFormat) and the typeless-package.jsonwarning — not feasible/desired without asourcebuffer in exsolve's architecture.Tests
Added 44 regression tests (suite: 33 → 77 passing, 1 skipped) across
test/internal-errors.test.ts,test/internal-get-format.test.ts, andtest/resolve-imports-exports.test.ts(+imports-pkg/exports-pkgfixtures). Both resolver fixes were confirmed by reverting them and observing the new tests fail.tsc,eslint, andprettierall pass.🤖 Generated with Claude Code
Summary by CodeRabbit
toString()output.data:URLs, with safer fallback rules for.jsand extensionless paths.imports/exportssubpath validation and conditional export fall-through behavior.imports/exportsmaps.