-
Notifications
You must be signed in to change notification settings - Fork 5.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
fix(npm): resolve node_modules dir relative to package.json instead of cwd #17885
Conversation
@@ -524,6 +535,14 @@ impl ConfigFile { | |||
/// Filenames that Deno will recognize when discovering config. | |||
const CONFIG_FILE_NAMES: [&str; 2] = ["deno.json", "deno.jsonc"]; | |||
|
|||
// todo(dsherret): in the future, we should enforce all callers | |||
// to provide a resolved path |
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.
What I've done in other projects is introduced a ResolvedPath
struct, then force resolving that once and passing it around.
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.
That's a very good idea, we should do the same
cli/tests/testdata/run/with_package_json/no_deno_json/sub_dir/main.js
Outdated
Show resolved
Hide resolved
} | ||
|
||
Ok(()) | ||
} |
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.
This was taken from elsewhere in the CLI.
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, great improvement. Thanks for looking into this!
No description provided.