Skip to content
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

cli: implement node --run <script-in-package-json> #52190

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 44 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -1807,6 +1807,50 @@ Only CommonJS modules are supported.
Use [`--import`][] to preload an [ECMAScript module][].
Modules preloaded with `--require` will run before modules preloaded with `--import`.

### `--run`

<!-- YAML
added: REPLACEME
-->

> Stability: 1.1 - Active development

This runs a specified command from a package.json's `"scripts"` object.
If no `"command"` is provided, it will list the available scripts.

`--run` prepends `./node_modules/.bin`, relative to the current
working directory, to the `PATH` in order to execute the binaries from
dependencies.

For example, the following command will run the `test` script of
the `package.json` in the current folder:

```console
$ node --run test
```

You can also pass arguments to the command. Any argument after `--` will
be appended to the script:

```console
$ node --run test -- --verbose
```

#### Intentional limitations

`node --run` is not meant to match the behaviors of `npm run` or of the `run`
commands of other package managers. The Node.js implementation is intentionally
more limited, in order to focus on top performance for the most common use
cases.
Some features of other `run` implementations that are intentionally excluded
are:

* Searching for `package.json` files outside the current folder.
* Prepending the `.bin` or `node_modules/.bin` paths of folders outside the
current folder.
* Running `pre` or `post` scripts in addition to the specified script.
* Defining package manager-specific environment variables.

### `--secure-heap=n`

<!-- YAML
Expand Down
74 changes: 74 additions & 0 deletions lib/internal/main/run.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
'use strict';
/* eslint-disable node-core/prefer-primordials */

// There is no need to add primordials to this file.
// `run.js` is a script only executed when `node --run <script>` is called.
const {
prepareMainThreadExecution,
markBootstrapComplete,
} = require('internal/process/pre_execution');
const { getPackageJSONScripts } = internalBinding('modules');
anonrig marked this conversation as resolved.
Show resolved Hide resolved
const { execSync } = require('child_process');
const { resolve, delimiter } = require('path');
const { escapeShell } = require('internal/shell');
const { getOptionValue } = require('internal/options');
const { emitExperimentalWarning } = require('internal/util');

prepareMainThreadExecution(false, false);
markBootstrapComplete();
emitExperimentalWarning('Task runner');

anonrig marked this conversation as resolved.
Show resolved Hide resolved
// TODO(@anonrig): Search for all package.json's until root folder.
const json_string = getPackageJSONScripts();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: jsonString (JS convesions and not C++/Python)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still in the incorrect case?


// Check if package.json exists and is parseable
if (json_string === undefined) {
process.exitCode = 1;
return;
}
const scripts = JSON.parse(json_string);
// Remove the first argument, which are the node binary.
const args = process.argv.slice(1);
const id = getOptionValue('--run');
let command = scripts[id];

if (!command) {
const { error } = require('internal/console/global');

error(`Missing script: "${id}"\n`);

const keys = Object.keys(scripts);
if (keys.length === 0) {
error('There are no scripts available in package.json');
} else {
error('Available scripts are:');
for (const script of keys) {
error(` ${script}: ${scripts[script]}`);
}
}
process.exit(1);
return;
}

const env = process.env;
const cwd = process.cwd();
const binPath = resolve(cwd, 'node_modules/.bin');

// Filter all environment variables that contain the word "path"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Filter all environment variables that contain the word "path"
// Filter all environment variables that case-insensitively equal "path"

const keys = Object.keys(env).filter((key) => /^path$/i.test(key));
const PATH = keys.map((key) => env[key]);

// Append only the current folder bin path to the PATH variable.
// TODO(@anonrig): Prepend the bin path of all parent folders.
const paths = [binPath, PATH].join(delimiter);
for (const key of keys) {
env[key] = paths;
}

// If there are any remaining arguments left, append them to the command.
// This is useful if you want to pass arguments to the script, such as
// `node --run linter -- --help` which runs `biome --check . --help`
if (args.length > 0) {
command += ' ' + escapeShell(args.map((arg) => arg.trim()).join(' '));
}
execSync(command, { stdio: 'inherit', env, shell: true });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Have you considered using spawn instead of execSync, which has limitations in buffer size?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was re-written in cpp 🙂

auto runner =
ProcessRunner(result, std::string(command_id), command, positional_args);
runner.Run();

void ProcessRunner::Run() {
if (int r = uv_spawn(loop_, &process_, &options_)) {
fprintf(stderr, "Error: %s\n", uv_strerror(r));
}
uv_run(loop_, UV_RUN_DEFAULT);
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @atlowChemi for clarifying!

37 changes: 37 additions & 0 deletions lib/internal/shell.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
'use strict';


// There is no need to add primordials to this file.
// `shell.js` is a script only executed when `node run <script>` is called.

const forbiddenCharacters = /[\t\n\r "#$&'()*;<>?\\`|~]/;

/**
* Escapes a string to be used as a shell argument.
*
* Adapted from `promise-spawn` module available under ISC license.
anonrig marked this conversation as resolved.
Show resolved Hide resolved
* Ref: https://github.com/npm/promise-spawn/blob/16b36410f9b721dbe190141136432a418869734f/lib/escape.js
* @param {string} input
*/
function escapeShell(input) {
anonrig marked this conversation as resolved.
Show resolved Hide resolved
// If the input is an empty string, return a pair of quotes
if (!input.length) {
return '\'\'';
}

// Check if input contains any forbidden characters
// If it doesn't, return the input as is.
if (!forbiddenCharacters.test(input)) {
return input;
}

// Replace single quotes with '\'' and wrap the whole result in a fresh set of quotes
return `'${input.replace(/'/g, '\'\\\'\'')}'`
// If the input string already had single quotes around it, clean those up
.replace(/^(?:'')+(?!$)/, '')
.replace(/\\'''/g, '\\\'');
}

module.exports = {
escapeShell,
};
4 changes: 4 additions & 0 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,10 @@ MaybeLocal<Value> StartExecution(Environment* env, StartExecutionCallback cb) {
return StartExecution(env, "internal/main/watch_mode");
}

if (!env->options()->run.empty()) {
return StartExecution(env, "internal/main/run");
}

if (!first_argv.empty() && first_argv != "-") {
return StartExecution(env, "internal/main/run_main_module");
}
Expand Down
40 changes: 40 additions & 0 deletions src/node_modules.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "base_object-inl.h"
#include "node_errors.h"
#include "node_external_reference.h"
#include "node_process-inl.h"
#include "node_url.h"
#include "permission/permission.h"
#include "permission/permission_base.h"
Expand Down Expand Up @@ -219,6 +220,21 @@ const BindingData::PackageConfig* BindingData::GetPackageJSON(
if (field_value == "commonjs" || field_value == "module") {
package_config.type = field_value;
}
} else if (key == "scripts") {
if (value.type().get(field_type)) {
return throw_invalid_package_config();
}
switch (field_type) {
case simdjson::ondemand::json_type::object: {
if (value.raw_json().get(field_value)) {
return throw_invalid_package_config();
}
package_config.scripts = field_value;
break;
}
default:
break;
}
}
}
// package_config could be quite large, so we should move it instead of
Expand Down Expand Up @@ -344,6 +360,28 @@ void BindingData::GetNearestParentPackageJSONType(
args.GetReturnValue().Set(Array::New(realm->isolate(), values, 3));
}

void BindingData::GetPackageJSONScripts(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I absolutely don't see the point of doing this from C++ "this late".

  • If all it does is read a JSON file it can live in JS and be roughly as fast.
  • If we do it in C++, why bootstrap everything, spin up an isolate and do all that? I figured the point of doing it in C++ is to do a simple (sync) read (ideally without needing a uv threadpool?), parse the json and execute the child process.

I'm not blocking of that but the approach here seems like the worst of both worlds?

Copy link
Member Author

@anonrig anonrig Apr 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll follow up and rewrite it in C++

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear I am not blocking this (and will lgtm, when the remaining doc fixes are fixed) - just trying to understand the motivation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

motivation: speed. more.

const FunctionCallbackInfo<Value>& args) {
Realm* realm = Realm::GetCurrent(args);
std::string_view path = "package.json";

THROW_IF_INSUFFICIENT_PERMISSIONS(
realm->env(), permission::PermissionScope::kFileSystemRead, path);

auto package_json = GetPackageJSON(realm, path);
if (package_json == nullptr) {
printf("Can't read package.json\n");
anonrig marked this conversation as resolved.
Show resolved Hide resolved
return;
} else if (!package_json->scripts.has_value()) {
printf("Can't read package.json \"scripts\" object\n");
return;
}

args.GetReturnValue().Set(
ToV8Value(realm->context(), package_json->scripts.value())
.ToLocalChecked());
}

void BindingData::GetPackageScopeConfig(
const FunctionCallbackInfo<Value>& args) {
CHECK_GE(args.Length(), 1);
Expand Down Expand Up @@ -424,6 +462,7 @@ void BindingData::CreatePerIsolateProperties(IsolateData* isolate_data,
"getNearestParentPackageJSON",
GetNearestParentPackageJSON);
SetMethod(isolate, target, "getPackageScopeConfig", GetPackageScopeConfig);
SetMethod(isolate, target, "getPackageJSONScripts", GetPackageJSONScripts);
}

void BindingData::CreatePerContextProperties(Local<Object> target,
Expand All @@ -440,6 +479,7 @@ void BindingData::RegisterExternalReferences(
registry->Register(GetNearestParentPackageJSONType);
registry->Register(GetNearestParentPackageJSON);
registry->Register(GetPackageScopeConfig);
registry->Register(GetPackageJSONScripts);
}

} // namespace modules
Expand Down
3 changes: 3 additions & 0 deletions src/node_modules.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class BindingData : public SnapshotableObject {
std::string type = "none";
std::optional<std::string> exports;
std::optional<std::string> imports;
std::optional<std::string> scripts;
std::string raw_json;

v8::Local<v8::Array> Serialize(Realm* realm) const;
Expand Down Expand Up @@ -60,6 +61,8 @@ class BindingData : public SnapshotableObject {
const v8::FunctionCallbackInfo<v8::Value>& args);
static void GetPackageScopeConfig(
const v8::FunctionCallbackInfo<v8::Value>& args);
static void GetPackageJSONScripts(
const v8::FunctionCallbackInfo<v8::Value>& args);

static void CreatePerIsolateProperties(IsolateData* isolate_data,
v8::Local<v8::ObjectTemplate> ctor);
Expand Down
3 changes: 3 additions & 0 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,9 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
&EnvironmentOptions::prof_process);
// Options after --prof-process are passed through to the prof processor.
AddAlias("--prof-process", { "--prof-process", "--" });
AddOption("--run",
"Run a script specified in package.json",
&EnvironmentOptions::run);
#if HAVE_INSPECTOR
AddOption("--cpu-prof",
"Start the V8 CPU profiler on start up, and write the CPU profile "
Expand Down
1 change: 1 addition & 0 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ class EnvironmentOptions : public Options {
bool heap_prof = false;
#endif // HAVE_INSPECTOR
std::string redirect_warnings;
std::string run;
std::string diagnostic_dir;
std::string env_file;
bool has_env_file_string = false;
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/run-script/.env
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
CUSTOM_ENV="hello world"
2 changes: 2 additions & 0 deletions test/fixtures/run-script/node_modules/.bin/ada

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/fixtures/run-script/node_modules/.bin/ada.bat

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions test/fixtures/run-script/node_modules/.bin/custom-env

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/fixtures/run-script/node_modules/.bin/custom-env.bat

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions test/fixtures/run-script/node_modules/.bin/positional-args

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions test/fixtures/run-script/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1",
"ada": "ada",
"ada-windows": "ada.bat",
"positional-args": "positional-args",
"positional-args-windows": "positional-args.bat",
"custom-env": "custom-env",
"custom-env-windows": "custom-env.bat"
}
}
14 changes: 14 additions & 0 deletions test/message/node_run_non_existent.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
'use strict';

require('../common');
const assert = require('node:assert').strict;
const childProcess = require('node:child_process');
const fixtures = require('../common/fixtures');

const child = childProcess.spawnSync(
process.execPath,
[ '--run', 'non-existent-command'],
{ cwd: fixtures.path('run-script'), encoding: 'utf8' },
);
assert.strictEqual(child.status, 1);
console.log(child.stderr);
10 changes: 10 additions & 0 deletions test/message/node_run_non_existent.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
Missing script: "non-existent-command"

Available scripts are:
test: echo "Error: no test specified" && exit 1
ada: ada
ada-windows: ada.bat
positional-args: positional-args
positional-args-windows: positional-args.bat
custom-env: custom-env
custom-env-windows: custom-env.bat