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

Conversation

nwalters512
Copy link
Contributor

@nwalters512 nwalters512 commented May 15, 2024

This PR fixes the issue that's been described at length in the following issues:

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. 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.

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.

@bengl
Copy link
Member

bengl commented May 31, 2024

@nwalters512 This LGTM but there are now conflicts. Can you rebase plz?

@nwalters512
Copy link
Contributor Author

@bengl done ✅

@bengl bengl merged commit 7fa4e9c into nodejs:main May 31, 2024
48 checks passed
@bengl bengl mentioned this pull request May 31, 2024
bengl added a commit that referenced this pull request May 31, 2024
$ git log --oneline --no-decorate v1.7.4..HEAD
9269d1f 1.8.0
74124e9 fix: handling of default and star exports (#85)
7fa4e9c Fix resolution of lib/register.js when using multiple loaders
(#76)
e7f05ca add node 22 to test matrix (#84)
fb0e393 feat: Add `Hook` named export (#88)
bef32f2 chore: don't create a package-lock file (#67)
bf3a4fb fix: Resolve re-exports of external modules (#78)
0d9f351 fix: Handle cyclical reference to current file (#83)
1e05e4b test: skip static-import on > v21 (#81)
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.

3 participants