Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix resolution of lib/register.js when using multiple loaders #76

Merged
merged 3 commits into from
May 31, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion hook.js
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,17 @@ function addIitm (url) {
}

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

async function resolve (specifier, context, parentResolve) {
// See github.com/DataDog/import-in-the-middle/pull/76.
if (specifier === iitmURL) {
Copy link
Member

Choose a reason for hiding this comment

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

This will only work if specifier is an absolute URL. It hasn’t been resolved yet, so this is the string passed to import.

What is the approach being pursued here? When IITM itself is being registered, short-circuit the chains? But this is ITTM, so this condition would only ever be true if ITTM is registered twice?

Copy link
Contributor Author

@nwalters512 nwalters512 May 15, 2024

Choose a reason for hiding this comment

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

Double-check what iitmURL actually refers to. It is not hook.js; it is lib/register.js, which is imported from generated code here:

https://github.com/DataDog/import-in-the-middle/blob/00b01fff1f5b69dd25e307593ec54d1d8abb4844/hook.js#L302

Note that in this case, the specifier templated into the generated code and passed to import is an absolute file:// URL.

Copy link
Contributor Author

@nwalters512 nwalters512 May 15, 2024

Choose a reason for hiding this comment

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

For slightly more context, the actual problem stems from the fact that import-in-the-middle ends up generating source for node:* imports via load(), and that this code imports absolute file URLs (lib/register.js). That's how we end up in a situation where parentURL is node:* and specifier is an absolute file:// URL.

return {
url: specifier,
shortCircuit: true
}
}

const { parentURL = '' } = context
const newSpecifier = deleteIitm(specifier)
if (isWin && parentURL.indexOf('file:node') === 0) {
Expand Down Expand Up @@ -262,7 +272,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)
}
}
Loading