Skip to content

Commit

Permalink
Fix resolution of lib/register.js when using multiple loaders (#76)
Browse files Browse the repository at this point in the history
This PR fixes the issue that's been described at length in the following
issues:

- getsentry/sentry-javascript#12011
- nodejs/node#52987

To summarize, `import-in-the-middle` can cause problems in the following
situation:

- A Node core module (`node:*`) is imported
- Another loader is added before `import-in-the-middle`
- That loader tries to resolve files that don't exist on disk

In practice, this occurs when using `import-in-the-middle` with code
that's being executed with
[`tsx`](https://github.com/privatenumber/tsx). `tsx` will try to resolve
`.js` files by also looking for the same file path but with a `.ts`
extension. In many cases, including the synthetic code that
`import-in-the-middle` generates to import `lib/register.js`, such a
corresponding `.ts` file does not exist.

The actual error arises from Node, which assumes that `parentURL` will
always be a `file://` URL when constructing an `ERR_MODULE_NOT_FOUND`
error; see the linked issue above. In the above scenario, the `.ts` file
that is being resolved does not exist, so such an error is created, and
`parentURL === 'node:*'`, so the failing case is triggered. It seems
like Node is receptive to changing that behavior, but in the meantime, I
was hoping to land this patch so that one doesn't have to wait for and
upgrade to a new version of Node.

The fix works by short-circuiting the resolution of `lib/register.js` so
that the other loader (that tries to resolve non-existent paths) is
never tried.
  • Loading branch information
nwalters512 authored May 31, 2024
1 parent e7f05ca commit 7fa4e9c
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 2 deletions.
12 changes: 11 additions & 1 deletion hook.js
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,19 @@ function addIitm (url) {

function createHook (meta) {
let cachedResolve
const iitmURL = new URL('lib/register.js', meta.url).toString()

async function resolve (specifier, context, parentResolve) {
cachedResolve = parentResolve

// See github.com/DataDog/import-in-the-middle/pull/76.
if (specifier === iitmURL) {
return {
url: specifier,
shortCircuit: true
}
}

const { parentURL = '' } = context
const newSpecifier = deleteIitm(specifier)
if (isWin && parentURL.indexOf('file:node') === 0) {
Expand Down Expand Up @@ -308,7 +319,6 @@ function createHook (meta) {
}
}

const iitmURL = new URL('lib/register.js', meta.url).toString()
async function getSource (url, context, parentGetSource) {
if (hasIitm(url)) {
const realUrl = deleteIitm(url)
Expand Down
2 changes: 1 addition & 1 deletion test/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ options (assuming they're run from the project root):

```
--require ./test/version-check.js
--experimental-loader ./test/generic-loader.mmjs
--experimental-loader ./test/generic-loader.mjs
```

The entire test suite can be run with `npm test`.
6 changes: 6 additions & 0 deletions test/multiple-loaders/multiple-loaders.test.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { register } from 'node:module'

register('./typescript-loader.mjs', import.meta.url)
register('../../hook.mjs', import.meta.url)

await import('node:util')
26 changes: 26 additions & 0 deletions test/multiple-loaders/typescript-loader.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/**
* This simulates what something like `tsx` (https://github.com/privatenumber/tsx)
* will do: it will try to resolve a URL with a `.js` extension to a `.ts` extension.
*
* Combined with the test case in the adjacent `multiple-loaders.test.mjs` file,
* this forces `import-in-the-middle` into what used to be a failure state: where
* `context.parentURL` is a `node:*` specifier and the `specifier` refers to a file
* that does not exist.
*
* See https://github.com/nodejs/node/issues/52987 for more details.
*/
export async function resolve (specifier, context, defaultResolve) {
if (!specifier.endsWith('.js') && !specifier.endsWith('.mjs')) {
return await defaultResolve(specifier, context)
}

try {
return await defaultResolve(specifier.replace(/\.m?js/, '.ts'), context)
} catch (err) {
if (err.code !== 'ERR_MODULE_NOT_FOUND') {
throw err
}

return await defaultResolve(specifier, context)
}
}

0 comments on commit 7fa4e9c

Please sign in to comment.