fix(ext/node): improve shelljs compat with managed npm execution #24912
Conversation
cli/cache/deno_dir.rs
Outdated
| @@ -67,7 +67,7 @@ impl DenoDir { | |||
| let gen_path = root.join("gen"); | |||
|
|
|||
| let deno_dir = Self { | |||
| root, | |||
| root: std::fs::canonicalize(root)?, | |||
There was a problem hiding this comment.
I think this will error when root doesn't exist. There is a function under cli/utils/fs.rs called canonicalize_path_maybe_not_exists that helps.
cli/factory.rs
Outdated
| } | ||
| } else { | ||
| Some(node_modules.clone()) | ||
| }); |
There was a problem hiding this comment.
As discussed in person: instead of doing this here, let's move this up into the resolve_node_modules_folder function in cli/args/mod.rs (
Line 1746 in d1b2a98
This requires having access to the deno dir at this point though. I think we could:
- Move the
DenoDirProviderstruct out of cli/factory.rs and instead create it withinCliOptions::newand store it onCliOptions. - Expose
DenoDirProvideronCliOptions. - Pass
DenoDirProviderinto theresolve_node_modules_folderfunction and then use it in that function. - Get and canonicalize the deno dir path inside
resolve_node_modules_folderin order to compare it against the canonicalized node_modules dir path.
Addressed the above points. Confirmed $ ./target/debug/deno run -A npm:create-docusaurus
✔ What should we name this site? … website
✔ Select a template below... › classic (recommended)
✔ Which language do you want to use? › JavaScript
[INFO] Creating new Docusaurus project...
✔ Select a package manager... › npm
[INFO] Installing dependencies with npm...
npm warn deprecated [email protected]: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
npm warn deprecated [email protected]: Rimraf versions prior to v4 are no longer supported
npm warn deprecated [email protected]: Glob versions prior to v9 are no longer supported
added 1212 packages, and audited 1213 packages in 1m
298 packages are looking for funding
run `npm fund` for details
found 0 vulnerabilities
[SUCCESS] Created website. |
| @@ -810,11 +812,14 @@ impl CliOptions { | |||
|
|
|||
| let maybe_lockfile = maybe_lockfile.filter(|_| !force_global_cache); | |||
| let root_folder = start_dir.workspace.root_folder_configs(); | |||
| let deno_dir_provider = | |||
| Arc::new(DenoDirProvider::new(flags.cache_path.clone())); | |||
There was a problem hiding this comment.
deno_dir_provider is not Deferred anymore. The Deferred defined in cli/factory.rs uses once_cell::unsync::OnceCell and not thread-safe, that is inconvenient for CliOptions. Should I create once_cell::sync::OnceCell-version of Deferred and wrap this deno_dir_provider? (That feels a bit overkill to me for now)
| // always auto-discover the local_node_modules_folder when a package.json exists | ||
| if let Ok(deno_dir) = deno_dir_provider.get_or_create() { | ||
| // `deno_dir.root` can be symlink in macOS | ||
| if let Ok(root) = canonicalize_path_maybe_not_exists(&deno_dir.root) { |
There was a problem hiding this comment.
I wonder if we should surface these errors when they happen? I guess they'll be surfaced later anyway.
This PR improves the Node.js compatibility in managed npm resolution mode.
Currently when an npm script executes the script next to it (
child.js), the package.json next to it is discovered as config file for it. As a result, Deno uses$DENO_DIR/npm/registry.npmjs.org/<package>/<version>/node_modulesas root of npm modules. As a result, the script (child.js) is considered Deno script, not a npm script. That leads to the errorReferenceError: require is not defined.This PR fixes it by disabling the discovery of
node_moduleswhen the main specifier is inside ofDENO_DIR.closes #22732
closes #24589