Repository navigation
fix: defer CJS transformation to the _compile patch in the async load hook - #45
Merged
Merged
Conversation
isaacs
reviewed
Jul 9, 2026
isaacs
left a comment
Contributor
There was a problem hiding this comment.
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.
Member
Author
Yes having this configurable only helps us for a single minor release so agreed. Not worth having this configurable. I'll rework the PR! |
transformCjs option to opt out of CJS transformation in the async load hook
isaacs
approved these changes
Jul 9, 2026
timfish
marked this pull request as ready for review
July 9, 2026 18:00
Merged
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The async
loadhook transformed CommonJS by returningsource, which makes Nodeevaluate the module on the synchronous
require(esm)bridge. On Node < 24.12 thatbridge throws
ERR_VM_MODULE_LINK_FAILUREwhenever the module's top-levelrequire()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 bynodejs/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_compiletransforms it instead. This costsnothing:
Module.registeris only used below 24.13, andModulePatchis alreadyrequired alongside it, since a CJS package's internal
require()calls never reach theESM resolve hook anyway. ESM is unaffected — the
loadhook still owns it. Thesynchronous
registerHookspath is unchanged and still transforms CommonJS itself,as it has no
_compilepatch to fall back on and doesn't use the bridge.