Skip to content

feat(core): compile ES module syntax to CommonJS where tsc would (#905) - #911

Open
darcyrush wants to merge 24 commits into
oxc-project:mainfrom
darcyrush:feat/commonjs-lowering-tsc-parity
Open

darcyrush wants to merge 24 commits into
oxc-project:mainfrom
darcyrush:feat/commonjs-lowering-tsc-parity

Conversation

@darcyrush

@darcyrush darcyrush commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

#908 and #910 are merged; this branch is up to date with main.

AI generated, but ran the fix against a full live CJS project. The project uses package.json type as commonjs with all file extensions as .ts running NestJS 12 (ESM) and tests mocked using nodes test module mock.method.

I needed to upgrade to TypeScript 7.0.2 from 5.9.3 which ts-node doesn't support. tsx and other don't support legacy TypeScript decorators (or rather emitDecoratorMetadata, which NestJS uses). I'm not aware of any other JIT that supports CJS projects needing emit decorator metadata on TS7+.

Closes #905

Problem

In a "type": "commonjs" package, a .ts file that uses import/export runs as an ES module under oxc-node, even when tsconfig compiles it to CommonJS. Its namespace object is then frozen, so mock.method() from node:test can't replace its exports. Under tsc it can.

package.json "type": "commonjs", tsconfig "module": "node20"
test.ts: import * as lib from "../src/lib"; mock.method(lib, "compute", …)

                    main                                    this PR   tsc
oxnode --test       TypeError: Cannot redefine property     pass      pass

Node.js' own type stripping doesn't run this file as an ES module either: it rejects it with SyntaxError: Cannot use import statement outside a module.

Fix

A file with ES module syntax is compiled to CommonJS the way tsc compiles it when all of these hold:

  • its package is "type": "commonjs"
  • its tsconfig module is commonjs, node16, node18, node20 or nodenext (any case)
  • its extension is ambiguous: .ts, .tsx, .js, .jsx or none
  • it uses neither import.meta nor top-level await, which have no CommonJS form
  • it declares none of the names the CommonJS wrapper or the helpers below use: exports, require,
    module, __filename, __dirname, __importDefault, __importStar, __exportStar

Anywhere else, such as a package with no "type", "type": "module", or an ES module setting, the file runs as an ES module, as before. Switching a project's "type" to "module" turns the compilation off.

The output has tsc's shape. It's built from the module runner transform's output:

  • An export of a local that is never reassigned is a plain data property (exports.a = void 0; … exports.a = a;). It's writable, so mock.method(namespace, "a") reaches every importer. A hoisted function is exported up front.
  • A reassigned local, a decorated class and a namespace are exports.a itself, as tsc emits them: live, writable, and undefined (not a TDZ error) when an import cycle reads them early. Re-exports stay live through getters, in the form cjs-module-lexer detects, defined after their require(). A reassigned local exported under two names, declared by destructuring or set to an anonymous function also gets a getter.
  • An anonymous default function is hoisted as default_1 and exported up front. A side-effect import is a bare require().
  • A named import is require()'s result. Default and namespace imports go through __importDefault/__importStar, export * through __exportStar, and import() stays import().
  • On the load-hook path the compiled source is returned with format: "commonjs", so an ES module importer gets the named exports.

The compiled code require()s ./lib.js where the source imported ./lib.js. So Module._resolveFilename in register.mjs falls back to the TypeScript source (.js → .ts/.tsx, .jsx → .tsx/.ts, .mjs → .mts, .cjs → .cts). When the output is missing, ./x.js also finds x.jsx and ./x.jsx finds x.js. For a TypeScript importer outside node_modules, it also prefers the source over compiled output sitting beside it, as the ES module resolver does since #840, and config.ts over config.json for ./config; a file inside node_modules keeps its compiled .js. A bare request tries tsconfig paths and baseUrl first, through the new tsconfigPathCandidates binding. The hook thread's helper tag is registered on the main thread too, so the compiled files can require() their helpers.

Under node16, node18 and nodenext, the resolve hook no longer reports every .ts file as an ES module; the package's "type" decides, as it does for tsc. #908 made that change and is merged.

Behaviour changes

These apply only to projects with "type": "commonjs" and one of the module settings above:

  • A file with import/export now runs as CommonJS. An ES module's default import of it is module.exports, with the file's default export at .default, the same as TypeScript types it under nodenext.
  • An ES module importer doesn't see names the file re-exports with export * from another TypeScript file: Node.js only follows re-exports into .js and .cjs files.
  • Under node16/node18/nodenext, a .ts file without module syntax is CommonJS, also when an ES module imports it.
  • An ES module importer's named imports are a snapshot taken when the file finishes loading, as for any CommonJS module.
  • The compiled file's dependencies resolve with the require condition, as under tsc; a package whose exports offers only import fails.
  • Node.js caches a relative require() per directory and request, so when a .js file and a .ts file in one directory both require("./lib.js") next to lib.ts, whichever asks first decides which file both get.
  • require() of an ES module from a compiled file loaded through the module hooks needs esm: populate separate cache for require(esm) in imported CJS nodejs/node#59679 (22.22.3 on the 22.x line). Earlier versions fail with TypeError: Cannot read properties of undefined (reading 'exports').

The README has a new "ES module syntax in a CommonJS package" section covering this.

Tests

  • Rust: commonjs_lowering covers each module setting, the output shape (reassigned exports, side-effect imports, the anonymous default function), the cases that stay ES modules (import.meta, reserved names, esnext, preserve, .mts, a package without "type"), and type-only or declared reserved names that still compile.
  • New packages/integrate-commonjs package ("type": "commonjs", module: nodenext, run with
    oxnode --test), 20 tests:
    • mocking a namespace export, a function and a class, with importers seeing the mock
    • live reassigned exports
    • default, star and CommonJS imports
    • .js names finding .ts, with the source preferred over compiled output beside it
    • an ES module importer's named and default imports
    • import cycles: hoisted functions, and undefined for a reassigned let, a decorated class, a namespace and a re-export read early
    • a namespace import of a CommonJS class keeping its static members
    • a tsconfig paths alias and the .js → .jsx fallback
    • stack trace lines
    • import.meta and top-level await staying ES modules
    • runtime helpers
  • commonjs.test.ts fails to load on main. helpers.test.ts passes on both and guards what must not change.
  • commonjs-lowering.spec.ts (vitest) covers paths and baseUrl, config.ts beside config.json, the .js/.jsx fallbacks, node_modules files and non-enumerable exports.
  • The Clarify module resolution with .ts files using import and package.json type commonjs #905 repro passes on Node.js 26.12.1 and 22.22.1. vp run test (643 vitest tests plus the three node --test packages), cargo test, clippy and fmt pass.

@darcyrush
darcyrush force-pushed the feat/commonjs-lowering-tsc-parity branch from 00d8ed3 to 053adbb Compare November 9, 2026 11:30
@Brooooooklyn

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-10T20:28:51.927084Z 868725a New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 053adbb0dc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/lib.rs Resolved
Comment thread packages/core/register.mjs Resolved
Comment thread packages/core/register.mjs Resolved
Comment thread src/lib.rs Resolved
Comment thread packages/core/register.mjs Resolved
@darcyrush

darcyrush commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor Author

Further to the LLM responses to the comments above, additional output;

Limitations of the fix

  • Explicitly named tsconfig with references (OXC_TSCONFIG_PATH / TS_NODE_PROJECT): the referenced project that holds the importer is matched by its directory. oxc_resolver matches on its internal paths base (baseUrl, or else the directory), which isn't public. They only differ when a referenced project sets baseUrl outside its own directory.
  • baseUrl after a failed alias: it's also tried when all of an alias's targets are missing, as oxc_resolver does. If the tsconfig's directory is a symlink, the same path can be tried twice before falling back, which only costs a failed lookup.
  • Extensionless rule only for TypeScript/JSX importers: that's the same gate as the existing source-first rule. A .js file with ES module syntax that gets compiled to CommonJS still gets Node.js' order for ./config (.json before .ts), though as an ES module it got TypeScript first. Hand-written CommonJS .js files keep Node.js' behaviour.
  • Directory requests: ./dir with dir/index.json beside dir/index.ts still follows Node.js' order.
  • Behaviour change: every bare, non-built-in require() from a project file now checks tsconfig paths first, so an alias with the same name as an installed package wins for require() too. Each such require() also makes one native call; the tsconfig lookup is cached.
  • Watch mode: the tsconfig reads the new binding makes are reported to the watch parent. The alias targets require() tries are not, whereas the ES module resolver reports the paths it tries. Creating a missing alias target therefore may not trigger a restart. I haven't checked this in practice.
  • Generated loaders: only the new export was added to index.js and index.d.ts. The committed WASI artifacts already lag behind (they lack freezeTsconfig), so I left them for CI to regenerate.

Out of scope / future changes

  • One resolver for require() and import: comments 1, 2, 3 and 5 all came from register.mjs copying the native resolver's rules by hand. Routing require() calls from project files through the native resolver would remove that copy, but it touches conditions, exports maps, symlinks and the hook thread on Node.js < 26.2. It's better as its own PR.
  • Rust unit tests for path_alias_candidates: it's only covered by integration tests for now.

DArcy Rush and others added 2 commits November 9, 2026 14:51
An ES module compiled to CommonJS resolves its imports through
`require()`, so it lost behaviour the ES module resolver gave it:

- try tsconfig `paths` and `baseUrl` before Node.js' own resolution,
  through the new `tsconfigPathCandidates` binding
- prefer `config.ts` over `config.json` for an extensionless request
- keep a package's compiled `.js` when the resolved file is inside
  `node_modules`
- fall back from `./x.js` to `x.jsx`, and from `./x.jsx` to `x.js`,
  without letting either win over a file that exists
- copy non-enumerable exports in `__importStar`, as tsc does

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@darcyrush
darcyrush force-pushed the feat/commonjs-lowering-tsc-parity branch from 977e97f to f4a08d9 Compare November 9, 2026 13:54
Brooooooklyn and others added 2 commits November 10, 2026 19:24
- Reassigned locals, decorated classes and namespaces are `exports.x` itself, as
  tsc emits them: live, writable, and `undefined` rather than a TDZ error when an
  import cycle reads them early. Re-exports are getters defined after their
  `require()`.
- An anonymous default function is hoisted as `default_1` and exported up front.
- A side-effect import is a bare `require()`.
- Only value bindings named like the CommonJS wrapper or the helpers keep a file
  an ES module; types, ambient declarations and type-only imports do not.
- Tests for cycles, a class's statics through a namespace import, `paths` and
  the `.jsx` fallback in a compiled file.
- README: the output's shape, cycles, snapshots for ES importers, the `require`
  condition, the per-directory `require()` cache, and known limits.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Brooooooklyn

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 19c19cd4b3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/core/register.mjs Resolved
Comment thread src/lib.rs Resolved
Comment thread src/lib.rs Resolved
…ceiver

- `export const a = 1, b = f();` assigns `exports.a` before `f()` runs, as
  tsc emits it, so an import cycle sees `a`.
- A reassigned export used as a template tag is called as `(0, exports.tag)`,
  as tsc calls it, so it gets no `this`.
- `require(".")` and `require("..")` are relative requests, so a TypeScript
  importer finds `index.ts` before a compiled `index.js` next to it, as for
  `./` and `../`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Brooooooklyn

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c135b3d23f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/core/register.mjs Resolved
Comment thread packages/core/register.mjs Resolved
Comment thread packages/core/register.mjs Resolved
…e ES module did

- A package `imports` entry or a self-reference through `exports` that names
  missing compiled output (`#value` -> `./value.js`, only `value.ts` on disk)
  finds its source through the native resolver, with the `require` condition.
- A `.js` file compiled from an ES module resolves an extensionless request
  TypeScript first, as it did as an ES module. It is told apart from
  hand-written CommonJS by `exports.__esModule`, which its first statement sets.
- A source chosen over compiled output goes back through Node.js' resolution,
  so a symlinked source gets its real path and loads once.
- test: the `--kill-timeout=300` watch test no longer expects the app's and
  oxnode's lines in one order, which two processes cannot promise.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Brooooooklyn

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4406e93e11

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/core/register.mjs Resolved
Comment thread packages/core/register.mjs Resolved
…fallback

- A package `imports` entry or self-reference that names missing output now
  takes the file Node.js mapped it to, which its error names, so every active
  condition (`--conditions=development`) applies, not a fixed list.
- A relative request Node.js cannot find asks the native resolver, which
  reports its other candidates to watch mode: creating `barrel/index.ts` for
  `import "./barrel"` restarts the app again.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Brooooooklyn

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a98adf2579

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/core/register.mjs Resolved
Comment thread src/lib.rs Resolved
Comment thread packages/core/register.mjs Resolved
… by the requested name

- `export const [a = 1, b = self.a] = v;` becomes
  `[exports.a = 1, exports.b = self.a] = v;`, as tsc emits it, so each binding
  is exported before the next default runs. Object patterns and rest elements
  too; the pattern's own reads go through `exports`.
- A package `imports` entry or a self-reference that maps to compiled output
  next to its source loads the source, as the ES module resolver did.
- The source is looked for beside the name a request reaches before Node.js
  follows a symlink there: `./alias.js` linking to `build/real.js` finds
  `alias.ts`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Brooooooklyn

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f1709b2e4a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/lib.rs Resolved
Comment thread packages/core/register.mjs Resolved
…ry a directory's main source

- The compiled output's prelude and helpers read `Object` and `Symbol`. A file
  that declares either at its top level would hand them its own binding before
  it is initialized, so such a file stays an ES module, as one declaring
  `require` or a helper's name does.
- For `./lib` whose `package.json` `main` names missing output, the source of
  `main` (`src/main.ts` for `./src/main.js`) is tried before the `index.js`
  Node.js falls back to, as the ES module resolver tried it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Brooooooklyn

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6d218b8eca

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/core/register.mjs Resolved
Comment thread packages/core/register.mjs Resolved
…e ES module order

- `./barrel` that Node.js resolves to `barrel/index.json` finds
  `barrel/index.ts` beside it first, as the ES module resolver did.
- An extensionless request takes the ES module resolver's extension order
  before the aliases of the extension Node.js added, so `./dep` beside
  `dep.ts`, `dep.tsx` and `dep.jsx` finds `dep.ts`, not `dep.tsx`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Brooooooklyn

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cd3b19f4eb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/core/register.mjs Resolved
…ect's baseUrl for aliases

- A `var` declared more than once is `exports.x` itself, as tsc emits it, so
  `var x = 1; export { x }; ...; var x;` exports `x` at its initializer, not
  after the last, empty declaration.
- With an explicitly named tsconfig that references a subproject, the
  `baseUrl` fallback after an alias's files are missing reads that
  subproject's `baseUrl`, as the alias candidates do, not the root's.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Brooooooklyn

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9ac9bbd8b0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/core/register.mjs Resolved
Node.js follows `./alias` to `real/` before it picks `index.json`, so the
requested directory is compared, and its `main` read, under the spelling
`resolved` has: the real path, or the link with `--preserve-symlinks`.
`index.ts` there now wins, as under the ES module resolver.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Brooooooklyn

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 03b9f03f9f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/lib.rs Resolved
Comment thread packages/core/register.mjs Resolved
…ted directory main

A top-level `var` that a loop header or block declares again is set
there too, so it stays a live getter instead of becoming `exports.x`
only at the top level.

A directory's `main` with a dot in its name (`./src/entry.prod`,
`./src.v1`) is not a file, so its extension order and index are probed
the way the ES module resolver probes them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Brooooooklyn

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0b9367a33b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/core/register.mjs Resolved
Comment thread src/lib.rs Resolved
…skip globals for default_N

`./lib` resolving to `lib/src/lib.cjs` named the directory, so the
extensionless probes no longer read `.cjs` as an extension Node.js added
and swap in `lib.ts`.

The name for an anonymous default function skips names the module uses
as globals, so `default_1()` inside it still calls the global, as `tsc`
picks `default_2`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Brooooooklyn

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 08788696bf

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/lib.rs Resolved
Comment thread packages/core/register.mjs Resolved
…be a member, and leave require.resolve paths to Node.js

A pattern is rewritten whole, so its names become `exports` members only
when each one can: not when another pattern binds a name nothing exports,
a block or a function declaration declares one again, or a function or
class takes its name from one (`var x = function () {}`, `[f = () => {}]`).
`var [x] = [1]; var [x, local] = [2, 3]` no longer panics.

`require.resolve(request, { paths })` returns what Node.js finds in those
directories: no tsconfig alias before it, and no source beside the caller
when nothing is found. `.` and `..` name the directory only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Brooooooklyn

Copy link
Copy Markdown
Member

@codex review

…ings

The output also carries stderr, where the WASI target prints an
`ExperimentalWarning`, so the test checks what the output contains.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3f0888a7bf

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/lib.rs Resolved
Comment thread packages/core/register.mjs Resolved
… ./foo.js's source before a foo.js directory

Only the runner's free `__vite_ssr_exports__` becomes `exports`; a
parameter or variable of that name keeps its reads.

`./foo.js` that Node.js completed with a `foo.js/` directory index finds
`foo.ts` first, as the ES module resolver does. `./foo.js/` still names
the directory.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Brooooooklyn

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fe3a3c0bad

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/lib.rs Resolved
Comment thread packages/core/register.mjs Resolved
Comment thread packages/core/register.mjs Resolved
Comment thread src/lib.rs Resolved
… and three resolution and naming cases

- A `var` declared again gets its getter after the first declaration, so
  an import cycle reads what the first initializer set.
- A parameter named `__vite_ssr_export_default__` keeps its reads.
- A directory's `main` that is a symlink finds the source beside the
  name `main` gives, as the ES module resolver does.
- An extensionless entry compiled to CommonJS (`bin/run`) resolves its
  extensionless requests TypeScript first, like a `.js` one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Brooooooklyn

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6a1da334da

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/lib.rs Resolved
Comment thread src/lib.rs Resolved
Comment thread packages/core/register.mjs Resolved
…o more shadowing and symlink cases

- A value named `exports` in any scope, a parameter included, keeps the
  file an ES module: the compiled `exports.x` inside a function would go
  to it.
- A parameter named `__vite_ssr_dynamic_import__` is called, not turned
  into `import()`.
- `./dep` whose `dep.js` links to `build/real.cjs` finds `dep.ts`, from
  the extension Node.js added rather than the link target's.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Brooooooklyn

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4d48275323

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/core/register.mjs Resolved
Comment thread src/lib.rs Resolved
…d find the source beside a symlinked index

The compiled output finds the module runner's helpers by their
`__vite_ssr_` names, so a file declaring a value of such a name in any
scope stays an ES module, as one declaring `exports` does. This covers
`const __vite_ssr_import__ = …` and replaces the per-name checks for
`__vite_ssr_exports__`, `__vite_ssr_export_default__` and
`__vite_ssr_dynamic_import__`.

A directory's index file that is a symlink Node.js followed finds the
source beside it (`lib/index.ts`), as a symlinked `main` already does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Brooooooklyn

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 121732ff65

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/lib.rs
Comment on lines +3101 to +3103
&& names.iter().all(|ident| export_names.get(ident.name.as_str()) == Some(&1))
{
destructured.extend(names.iter().map(|ident| ident.name.as_str()));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Publish aliased destructured exports as their bindings initialize

In a lowered module containing import * as self from './same'; export const [a = 1, b = self.a]: number[] = []; export { a as alias };, b becomes undefined, whereas native ESM and tsc both produce 1. The additional export name makes this check reject the entire pattern, and the later !written fallback publishes its exports only after the whole declaration completes. This single-declaration case is not covered by the earlier redeclared-var getter fix. Preserve per-binding publication when a destructured binding has multiple export names so later defaults can observe earlier initialized exports.

Useful? React with 👍 / 👎.

Comment on lines +285 to +288
} else if (mapsToProjectFile(request, parent, resolved)) {
// A package `imports` entry or a self-reference that maps to compiled output beside
// its source: the ES module resolver tried the source first after mapping.
source = sourceBeside(resolved);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Probe mapped source names before following package-import symlinks

With default symlink handling, a lowered TypeScript import of #dep runs compiled output when the package's imports maps it to ./dep.js, that file links to build/real.js, and dep.ts exists beside the link. A focused check of the extracted resolver hook on Node 24.15.0 returns build/real.js for #dep but correctly returns dep.ts for ./dep.js; this is fresh evidence that the bare-mapping branch bypasses the earlier requested-name fix. Probe source aliases beside the mapped target's original spelling before following its link, rather than only beside the canonical result, to preserve the previous source_order selection.

Useful? React with 👍 / 👎.

Comment thread packages/core/register.mjs Resolved
…mpletion Node.js finds

`./dep.js` finds `dep.ts` first whatever Node.js completed it with, a
`dep.js.json` file as well as a `dep.js/` directory, as the ES module
resolver does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 868725a173

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

source =
(DIRECTORY_ONLY.test(request)
? undefined
: sourceBeside(resolvePath(dirname(parent.filename), request))) ??

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Honor JSX fallbacks before accepting completed output filenames

The fresh case after the .ts completion fix is a lowered ./dep.js import with only dep.jsx and dep.js.json present: a Node 24 probe of the current hook returns the JSON file, whereas source_order tries the .jsx alias after the missing .js. This original-name probe uses SOURCES_BEFORE_OUTPUT, which excludes .jsx, and Node's successful filename completion bypasses the missing-output fallback entirely. When the requested output itself is absent, probe MISSING_OUTPUT_SOURCES before accepting the completed filename so lowering does not silently execute a different module.

Useful? React with 👍 / 👎.

Comment on lines +487 to +489
return index === undefined
? undefined
: (extensionlessSource("index", index) ?? sourceBeside(index));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Prefer the declared main's source over a symlinked index fallback

For a lowered ./lib import, if lib/package.json declares main: './src/main.js', only src/main.ts exists for that entry, and Node falls back to lib/index.js -> ../build/real.js beside lib/index.ts, this branch selects lib/index.ts. A Node 24 probe of the current hook reproduces that selection, while TypeScript resolution selects lib/src/main.ts, consistent with the existing source-order main probing. The new index-symlink probe runs before directoryMainSource, and Node's canonical result also lies outside the directory that helper checks. Probe the declared main's source before substituting a source for the fallback index so lowering preserves the intended entry point.

Useful? React with 👍 / 👎.

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.

Clarify module resolution with .ts files using import and package.json type commonjs

2 participants