-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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(node): better detection for when to surface node resolution errors #24653
fix(node): better detection for when to surface node resolution errors #24653
Conversation
@@ -0,0 +1,2 @@ | |||
error: [ERR_MODULE_NOT_FOUND] Cannot find module 'file:///[WILDLINE]/node_modules/package/index.js' imported from 'file:///[WILDLINE]/bad_import.ts' | |||
at file:///[WILDLINE]/bad_import.ts:1:16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now get node resolution errors here. This demonstrates a simple case, but imagine if this was something like exports not being resolved or the like. One issue is that the imported from '...'
is repetitive because deno graph provides this information. We may want to consider removing the referrer from error messages and then only adding it when necesasry (ex. when importing esm with require
)
Before:
error: Cannot find "package"
at file:///V:/deno/tests/specs/node/byonm_phantom_dep_res_failure/bad_import.ts:1:16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine for this pass 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
concat!( | ||
"Could not resolve \"{}\", but found it in a package.json. ", | ||
"Deno expects the node_modules/ directory to be up to date. ", | ||
"Did you forget to run `npm install`?" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention DENO_FUTURE=1 deno install
here as well, as per the Slack thread
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's land in a separate pr.
@@ -0,0 +1,2 @@ | |||
error: [ERR_MODULE_NOT_FOUND] Cannot find module 'file:///[WILDLINE]/node_modules/package/index.js' imported from 'file:///[WILDLINE]/bad_import.ts' | |||
at file:///[WILDLINE]/bad_import.ts:1:16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine for this pass 👍
No description provided.