Skip to content

Commit

Permalink
fix(node): Fix --allow-scripts with no deno.json (#24533)
Browse files Browse the repository at this point in the history
We would resolve the wrong package.json, resulting in an inability to
run CJS (or other node-mode) scripts
  • Loading branch information
nathanwhit authored Jul 15, 2024
1 parent 29186d7 commit 04f9db5
Show file tree
Hide file tree
Showing 10 changed files with 82 additions and 20 deletions.
6 changes: 3 additions & 3 deletions cli/args/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -722,15 +722,15 @@ pub enum NpmProcessStateKind {
Byonm,
}

const RESOLUTION_STATE_ENV_VAR_NAME: &str =
pub(crate) const NPM_RESOLUTION_STATE_ENV_VAR_NAME: &str =
"DENO_DONT_USE_INTERNAL_NODE_COMPAT_STATE";

static NPM_PROCESS_STATE: Lazy<Option<NpmProcessState>> = Lazy::new(|| {
let state = std::env::var(RESOLUTION_STATE_ENV_VAR_NAME).ok()?;
let state = std::env::var(NPM_RESOLUTION_STATE_ENV_VAR_NAME).ok()?;
let state: NpmProcessState = serde_json::from_str(&state).ok()?;
// remove the environment variable so that sub processes
// that are spawned do not also use this.
std::env::remove_var(RESOLUTION_STATE_ENV_VAR_NAME);
std::env::remove_var(NPM_RESOLUTION_STATE_ENV_VAR_NAME);
Some(state)
});

Expand Down
29 changes: 16 additions & 13 deletions cli/npm/managed/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -518,22 +518,25 @@ impl ManagedCliNpmResolver {
}
}

fn npm_process_state(
snapshot: ValidSerializedNpmResolutionSnapshot,
node_modules_path: Option<&Path>,
) -> String {
serde_json::to_string(&NpmProcessState {
kind: NpmProcessStateKind::Snapshot(snapshot.into_serialized()),
local_node_modules_path: node_modules_path
.map(|p| p.to_string_lossy().to_string()),
})
.unwrap()
}

impl NpmResolver for ManagedCliNpmResolver {
/// Gets the state of npm for the process.
fn get_npm_process_state(&self) -> String {
serde_json::to_string(&NpmProcessState {
kind: NpmProcessStateKind::Snapshot(
self
.resolution
.serialized_valid_snapshot()
.into_serialized(),
),
local_node_modules_path: self
.fs_resolver
.node_modules_path()
.map(|p| p.to_string_lossy().to_string()),
})
.unwrap()
npm_process_state(
self.resolution.serialized_valid_snapshot(),
self.fs_resolver.node_modules_path().map(|p| p.as_path()),
)
}

fn resolve_package_folder_from_package(
Expand Down
19 changes: 16 additions & 3 deletions cli/npm/managed/resolvers/local.rs
Original file line number Diff line number Diff line change
Expand Up @@ -282,8 +282,12 @@ fn resolve_baseline_custom_commands(
custom_commands
.insert("npm".to_string(), Rc::new(crate::task_runner::NpmCommand));

custom_commands
.insert("node".to_string(), Rc::new(crate::task_runner::NodeCommand));
custom_commands.insert(
"node".to_string(),
Rc::new(crate::task_runner::NodeCommand {
force_node_modules_dir: true,
}),
);

custom_commands.insert(
"node-gyp".to_string(),
Expand Down Expand Up @@ -687,7 +691,16 @@ async fn sync_resolution_with_fs(
&deno_local_registry_dir,
)?;
let init_cwd = lifecycle_scripts.initial_cwd.as_deref().unwrap();
let process_state = crate::npm::managed::npm_process_state(
snapshot.as_valid_serialized(),
Some(root_node_modules_dir_path),
);

let mut env_vars = crate::task_runner::real_env_vars();
env_vars.insert(
crate::args::NPM_RESOLUTION_STATE_ENV_VAR_NAME.to_string(),
process_state,
);
for (package, package_path, scripts_run_path) in packages_with_scripts {
// add custom commands for binaries from the package's dependencies. this will take precedence over the
// baseline commands, so if the package relies on a bin that conflicts with one higher in the dependency tree, the
Expand All @@ -710,7 +723,7 @@ async fn sync_resolution_with_fs(
task_name: script_name,
script,
cwd: &package_path,
env_vars: crate::task_runner::real_env_vars(),
env_vars: env_vars.clone(),
custom_commands: custom_commands.clone(),
init_cwd,
argv: &[],
Expand Down
8 changes: 7 additions & 1 deletion cli/task_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,9 @@ impl ShellCommand for NpmCommand {
}
}

pub struct NodeCommand;
pub struct NodeCommand {
pub force_node_modules_dir: bool,
}

impl ShellCommand for NodeCommand {
fn execute(
Expand All @@ -191,6 +193,9 @@ impl ShellCommand for NodeCommand {
.execute(context);
}
args.extend(["run", "-A"].into_iter().map(|s| s.to_string()));
if self.force_node_modules_dir {
args.push("--node-modules-dir=true".to_string());
}
args.extend(context.args.iter().cloned());

let mut state = context.state;
Expand Down Expand Up @@ -303,6 +308,7 @@ impl ShellCommand for NodeModulesFileRunCommand {
let mut args = vec![
"run".to_string(),
"--ext=js".to_string(),
"--node-modules-dir=true".to_string(),
"-A".to_string(),
self.path.to_string_lossy().to_string(),
];
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"name": "@denotest/lifecycle-scripts-cjs",
"version": "1.0.0",
"scripts": {
"preinstall": "echo preinstall && node preinstall.js",
"install": "echo install && cli-cjs 'hello from install script'"
},
"dependencies": {
"@denotest/bin": "1.0.0"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
const inspect = require('util').inspect;

inspect({
"preinstall": "script"
});
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
if ("Deno" in globalThis && typeof globalThis.Deno === 'object') {
require('./helper.js');
console.log('deno preinstall.js');
} else {
console.log('node preinstall.js');
Expand Down
13 changes: 13 additions & 0 deletions tests/specs/npm/lifecycle_scripts/__test__.jsonc
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,19 @@
"output": ""
}
]
},
"lifecycle_scripts_no_deno_json": {
"tempDir": true,
"steps": [
{
"args": ["eval", "Deno.removeSync('deno.json')"],
"output": ""
},
{
"args": "cache --allow-scripts --node-modules-dir=true no_deno_json.js",
"output": "no_deno_json.out"
}
]
}
}
}
1 change: 1 addition & 0 deletions tests/specs/npm/lifecycle_scripts/no_deno_json.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import {} from "npm:@denotest/[email protected]";
9 changes: 9 additions & 0 deletions tests/specs/npm/lifecycle_scripts/no_deno_json.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
Download http://localhost:4260/@denotest/lifecycle-scripts-cjs
Download http://localhost:4260/@denotest/bin
Download http://localhost:4260/@denotest/lifecycle-scripts-cjs/1.0.0.tgz
Download http://localhost:4260/@denotest/bin/1.0.0.tgz
Initialize @denotest/[email protected]
Initialize @denotest/[email protected]
preinstall
install
hello from install script

0 comments on commit 04f9db5

Please sign in to comment.