-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
import hook incompatible with tsx
#12011
Comments
The full stack trace is:
And here is the bottom of the call stack. Maybe the loader hook added by |
Looking at the stack trace, although
I guess it's not handling the params the iitm adds to the url? So I've opened an issue there: |
tsx
@timfish thanks for investigating this; your small reproduction that used just Changing that to just Specifically, it looks like the issue is that this URL with
|
Tracing the resolving of the otel hooking, it looks like Adding the query to the parent URL should be fine as this is still a valid URL. For example, the following code runs fine under both node and tsx: import { register } from "node:module";
register(
new URL(
`data:application/javascript;base64,${Buffer.from(
`
export async function resolve(specifier, context, nextResolve) {
if (context.parentURL) {
context.parentURL += "?some=query";
}
console.log("resolve", specifier, context);
return nextResolve(specifier, context);
}
`
).toString("base64")}`
),
import.meta.url
);
await import("node:util"); and outputs:
|
Ah, I just re-read this and it makes sense that this is the cause. |
I think you make have just realized this, but I'll finish this comment just so we're both on the same page. Your example is subtly different from what's happening in the failure case. In your example, const { fileURLToPath } = require('url');
fileURLToPath('node:util?foo=bar'); The above errors out exactly the same as the failure case with
|
You're right, but I think the issue isn't the query string, it's more that |
For example, when the minimal reproduction is run through plain old Node: import { register } from "node:module";
register("import-in-the-middle/hook.mjs", import.meta.url);
await import("node:util"); The iitm resolve function with this added:
Results in:
|
All great observations! I managed to make a very isolated reproduction: https://github.com/nwalters512/register-hook-playground It uses neither
It turns on that the query params were a red herring of sorts. The presence of it actually triggers another behavior in This puts us in a situation that Node would normally never expect to encounter: export async function resolve(specifier, context, parentResolve) {
specifier = 'file:///dost-not-exist.mjs';
context.parentURL = 'node:util';
const url = await parentResolve(specifier, context, parentResolve);
} This is obviously very contrived, but it reproduces the problem very simply: Node's I'm feeling pretty confident that this is a bug in |
What I don't yet fully understand is why iitm works with Node as it's using the same For example these are the logs from
It appears to handle the node: URLs without error! From the stack trace, we can see that when tsx is involved, it's called after iitm, ie. However, my console logs from tsx resolve aren't being outputted before the exception so it's really hard to see if/how the parameters are being modified before they hit the default resolver. My best guess is that they're not getting flushed from the loader hooks thread and there's no obvious way to debug it otherwise. |
I think I have an answer: this only happens when the file that we're trying to resolve with - const iitmURL = new URL('lib/register.mjs', import.meta.url).toString();
+ const iitmURL = new URL('register.mjs', import.meta.url).toString(); Note that the updated version points to a file that actually exists; when that happens, there's no error! So... maybe this is a bug in Node? If nothing else, it's unexpected behavior. I think he expected behavior in the does-not-exist case is that we'd get a export async function resolve(specifier, context, parentResolve) {
await parentResolve('file:///does-not-exist.mjs', context, parentResolve);
} Which errors with the following:
Aside: |
So to summarize, the combination of
This is a perfect storm that produces unexpected behavior in Node's own resolution process. |
Ah got it, the error is being thrown from here. It's throwing while it's trying to create the error message: |
Node tries to check if the If we report this to Node they might want to throw on all I'll open an issue there first and explain the detail... |
Excellent find! Sounds good; I'll check out the issue once you make it. |
I opened a PR to fix this in |
Maybe it would be good idea for Sentry to fork iitm and launch it as their own package. DDs shipping cadedance has not been particularly frequent, and this is going to be a major bottleneck for ESM adoption. |
The plan is for it to move to the Node org: |
Node org is equally slow to make releases. If this is going to be a bottleneck to Sentry v8 adoption, you want to have more control over it. It is small enough package to make little difference even if there are multiple versions of it maintained, i.e. a copy can be incubated under Sentry and if Node.js wants to merge upstream, let them do it. |
Normally, I'd agree, it would be nice if we could fork it. But sadly, in this case we can't do this because import-in-the-middle is a dependency of all the other opentelemetry instrumentation, which would not use our fork :( So we need to make fixes upstream there and live with the slower cadence 😬 |
This comment was marked as outdated.
This comment was marked as outdated.
@timfish what ultimately fixed things for me was actually #12043; we don't use performance instrumentation, so the error went away when Sentry stopped trying to unnecessarily instrument modules. This is of course not a general solution, and yours might very well be a reasonable bandaid for anyone who is using both |
There are multiple outstanding PRs for Until they make it to a release, I've combined these into a patch that can be used with If anyone can confirm that this fixes their issues that would be great! |
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.
Does the fact that this PR is closed means that now Sentry is compatible with tsx? |
This issue is still open #12357, but this specific issue is fixed. Just needs to be released - that will come out tomorrow most likely. |
Hello, we've just released v8.8.0, which should hopefully resolve this ESM problem. Let us know if you updated and are still running into problems! |
This issue might be fixed, but unfortunately
|
resolves #12242 (although there are still some follow ups) https://github.com/open-telemetry/opentelemetry-js/releases/tag/v1.25.0 I think this lockfile looks correct, but lmk if this feels off. resolves #12011 resolves #12059 resolves #12154 resolves #12237 resolves nodejs/import-in-the-middle#77 cc @mohd-akram
Is there an existing issue for this?
How do you use Sentry?
Sentry Saas (sentry.io)
Which SDK are you using?
@sentry/node
SDK Version
8.0.0
Framework Version
No response
Link to Sentry event
No response
SDK Setup
Steps to Reproduce
I've created a minimal reproduction here: https://github.com/nwalters512/sentry-v8-tsx-error-repro
yarn
yarn tsx src/index.ts
TypeError [ERR_INVALID_URL_SCHEME]: The URL must be of scheme file
Several observations that are hopefully helpful to y'all:
This only occurs when
"allowJs": true
is set intsconfig.json
. As Sentry isn't concerned with this, that made me think this problem might actually be intsx
. However...When adding instrumentation directly with the
@opentelemetry/*
packages, everything works fine. This is reproducible by making the following change tosrc/index.ts
:Given this, I'm strongly inclined to believe that this is an issue with the way in which Sentry is using OpenTelemetry.
This only breaks for core Node modules like
util
,fs
, etc. Importing other modules works fine. For instance, the following change tosrc/index.ts
makes it work without erroring:This only breaks for dynamic imports. For instance, the following change to
src/index.ts
makes it work without erroring:Expected Result
I would expect the process to complete successfully and log
util imported!
.Actual Result
The process errors out while importing
util
and does not printutil imported!
.The text was updated successfully, but these errors were encountered: