Skip to content

Deno panic when running node module #22671

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

Closed
NfNitLoop opened this issue Mar 2, 2024 · 3 comments · Fixed by #24727
Closed

Deno panic when running node module #22671

NfNitLoop opened this issue Mar 2, 2024 · 3 comments · Fixed by #24727
Assignees
Labels
bug Something isn't working correctly node compat

Comments

@NfNitLoop
Copy link

Here reporting a panic as requested by the CLI.

> deno run --allow-env --allow-read=. --allow-sys npm:fast-cli
✅ Granted read access to <main_module>.

============================================================
Deno has panicked. This is a bug in Deno. Please report this
at https://github.com/denoland/deno/issues/new.
If you can reliably reproduce this panic, include the
reproduction steps and re-run with the RUST_BACKTRACE=1 env
var set and include the backtrace in your report.

Platform: windows x86_64
Version: 1.41.1
Args: ["deno", "run", "--allow-env", "--allow-read=.", "--allow-sys", "npm:fast-cli"]

thread 'main' panicked at C:\a\deno\deno\ext\node\ops\require.rs:107:6:
called `Result::unwrap()` on an `Err` value: InvalidPath("C:Users\\codyc\\AppData\\Local\\deno\\npm\\registry.npmjs.org\\fast-cli\\3.2.0")
stack backtrace:
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

The path C:\Users\codyc\AppData\Local\deno\npm\registry.npmjs.org\fast-cli\3.2.0 does in fact exist. But I bet the problem is that there's no \\ after the C: in the invalid path in the error message.

@dsherret dsherret added bug Something isn't working correctly node compat labels Mar 3, 2024
@dsherret dsherret self-assigned this Mar 3, 2024
@dsherret
Copy link
Member

This one is complicated to fix. The underlying issue is that in Deno, all resolved modules are represented as file urls. This package transitively uses the https://www.npmjs.com/package/caller-path package, which uses https://www.npmjs.com/package/callsites, and that returns v8 callsites (https://v8.dev/docs/stack-trace-api#customizing-stack-traces). In Deno, callsite.getFileName() returns a file url, but in Node it returns a file url. On linux, this isn't much of a problem, but on Windows it will more frequently lead to bugs like the one above.

I'm not sure how to fix this. Maybe callsite.getFileName() should always return a path for files instead of a url or only do this when the caller is in an npm package, but I'm not sure.

@NfNitLoop
Copy link
Author

In Deno, callsite.getFileName() returns a file url, but in Node it returns a file url

I assume these were supposed to be different? 😅

Thanks for the update! FYI: this is not a blocker for me, I've moved on and don't even remember what I was doing to find it. I just reported it since your CLI asked me to. 😊

@marvinhagemeister
Copy link
Contributor

Took a look into this and the main issue is the import-jsx package, specifically the older version 4.0.1. Newer versions have different problems, but this one in particular does this to determine the callsite of the original function call and flags it as being a JSX module internally.

const modulePath = resolveFrom(path.dirname(callerPath()), moduleId);
// ...snip
const m = require(modulePath)

To fix this issue a small patch needs to be applied to the either the caller-calliste module or the caller-path module:

  'use strict';
  const callerCallsite = require('caller-callsite');

  module.exports = ({depth = 0} = {}) => {
    const callsite = callerCallsite({depth});
-   return callsite && callsite.getFileName();
+   return callsite && callsite.getFileName().replace(/^file:\/\//, "");
  };

With that the module works, although puppeteer seems to crash on shutdown. But the whole speed checking works:

Screenshot 2024-04-16 at 19 40 52

That said diving into the internals of fast-cli is a bit eyebrow raising. They should've simply pre-transpiled the JSX and shipped that. Requiring a whole React-based terminal renderer to print a single line and update that feels like cracking a walnut with a sledgehammer. It's certainly an artefact of its time.

dsherret pushed a commit that referenced this issue Jul 26, 2024
Adds support for `npm:bindings` and `npm:callsites` packages because of
changes in
denoland/deno_core#838.

This `deno_core` bump causes us to stop prepending `file://` scheme for
locations
in stack traces that are for local files.

Fixes #24462 , fixes
#22671 , fixes
#15717 , fixes
#19130 , fixes
WiseLibs/better-sqlite3#1205 , fixes
WiseLibs/better-sqlite3#1034 , fixes
#20936

---------

Co-authored-by: Nathan Whitaker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly node compat
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants