Skip to content

fix: defer CJS transformation to the _compile patch in the async load hook - #45

Merged
isaacs merged 4 commits into
mainfrom
fix/async-cjs-opt-out
Jul 13, 2026
Merged

isaacs merged 4 commits into
mainfrom
fix/async-cjs-opt-out

Conversation

@timfish

@timfish timfish commented Jul 9, 2026 •

Copy link
Copy Markdown
Member

The async load hook transformed CommonJS by returning source, which makes Node
evaluate the module on the synchronous require(esm) bridge. On Node < 24.12 that
bridge throws ERR_VM_MODULE_LINK_FAILURE whenever the module's top-level require()
chain reaches an ES module — the real-world case being koa → is-generator-function
→ generator-function. It's a Node bug (nodejs/node#59666), fixed in 24.12.0 by
nodejs/node#60380 and not backported to 22.x or 20.x.

The async hook now returns CommonJS untouched, so Node loads it through the ordinary
CommonJS loader where ModulePatch's _compile transforms it instead. This costs
nothing: Module.register is only used below 24.13, and ModulePatch is already
required alongside it, since a CJS package's internal require() calls never reach the
ESM resolve hook anyway. ESM is unaffected — the load hook still owns it. The
synchronous registerHooks path is unchanged and still transforms CommonJS itself,
as it has no _compile patch to fall back on and doesn't use the bridge.

@timfish
timfish requested review from bizob2828 and isaacs July 9, 2026 16:16

@isaacs isaacs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My main suggestion is that I think we can get away with not having the option. Just always transform CJS in sync mode, and never in async mode. It's slightly extra work in node 24.12, but that's fine, and no other node versions are in between the two flags.

Comment thread hook.mjs Outdated
Comment thread README.md Outdated
Comment thread README.md
Comment thread hook.mjs Outdated
@timfish

timfish commented Jul 9, 2026 •

Copy link
Copy Markdown
Member Author

only actually relevant in the 24.x branch at 24.12, right?

Yes having this configurable only helps us for a single minor release so agreed. Not worth having this configurable. I'll rework the PR!

@timfish timfish changed the title feat: add transformCjs option to opt out of CJS transformation in the async load hook fix: defer CJS transformation to the _compile patch in the async load hook Jul 9, 2026
@timfish
timfish requested a review from isaacs July 9, 2026 17:43
@timfish
timfish requested a review from jsumners-nr July 9, 2026 17:58
@timfish
timfish marked this pull request as ready for review July 9, 2026 18:00
@timfish
timfish requested a review from Qard July 9, 2026 18:01
@timfish timfish mentioned this pull request Jul 10, 2026
@isaacs
isaacs merged commit 2ac752a into main Jul 13, 2026
8 checks passed
@isaacs
isaacs deleted the fix/async-cjs-opt-out branch July 13, 2026 00:59
s1gr1d added a commit to getsentry/sentry-javascript that referenced this pull request Jul 16, 2026
…n integration (#22146)

Adds `koaChannelIntegration` in `@sentry/server-utils` for injecting
orchestrion channels into `koa`.

A subscriber wraps each registered layer in a span-creating proxy.
Span-helpers are ported from the vendored instrumentation, preserving
span names (but adapting to [new
conventions](getsentry/sentry-conventions#472)).

Also upgraded `@apm-js-collab/tracing-hooks` to get this:
apm-js-collab/tracing-hooks#45 to be released
(lets us actually patch `koa` - see 1. iteration below).



<details>
<summary>1. iteration</summary>

One thing to know for review: **we instrument `koa-compose`, not `use`
from koa.** koa's `use` lives in koa's main entry
(`lib/application.js`), and transforming a package's main entry forces
its top-level `require` chain through Node's `require(esm)` bridge,
which throws on Node < 24.13.

1. Orchestrion instruments by rewriting a module's source at load time
(via the ESM load hook).
2. `use` lives in koa's main entry (`lib/application.js`), so to
instrument it we transform that file. But transforming a main entry
pulls its whole top-level `require` chain into the loader's handling -->
and that changes how those `require`s are loaded (through the
ESM→CommonJS translator, not the normal sync require path).
3. `koa` is CJS (but support ESM). When `import`ing koa, it loads a shim
that loads the CJS code: `import 'koa'` → `dist/koa.mjs` (a ESM shim) →
`import '../lib/application.js'` (CJS). That CJS entry has a top-level
`require('is-generator-function')`.
4. `is-generator-function` → `require('generator-function')`, and
`generator-function` points at an `.mjs` file, which in turn imports
`./index.js`.
--> So the top-level `require` in koa's `application.js` (CJS) becomes
`require(esm)` of an ESM file importing a CJS file.
5. On Node < 24.13 (we pin 20.19.5), the loader can't pre-link this
dual-package shape into the `require(esm)` cache, so it throws `request
for './index.js' is not in cache`.

The failing chain:
```
koa/lib/application.js (CJS)
  └─ require('is-generator-function')        (CJS)
       └─ require('generator-function')      → require(esm) → require.mjs (ESM)
            └─ import './index.js'            (ESM importing CJS)
```

`koa-compose` is koa's zero-dependency dispatch engine, so it's safe to
transform, and `compose(app.middleware)` sees the same layers `use`
would. Since `@koa/router` also calls `compose` per request, the
subscriber uses `getActiveSpan()` to only wrap at app startup (no active
span) and skip the per-request router composition.</details>





Closes #20758

Linear:
https://linear.app/getsentry/issue/JS-2409/rewrite-opentelemetryinstrumentation-koa-to-orchestrion
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