Skip to content

cli: add support to bin scripts #58172

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
199 changes: 161 additions & 38 deletions src/node_task_runner.cc
Original file line number Diff line number Diff line change
Expand Up @@ -291,57 +291,180 @@ void RunTask(const std::shared_ptr<InitializationResultImpl>& result,
return;
}

// If package_json object doesn't have "scripts" field, throw an error.
Copy link
Member

Choose a reason for hiding this comment

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

Please add some comments :-)

simdjson::ondemand::object scripts_object;
if (main_object["scripts"].get_object().get(scripts_object)) {
fprintf(
stderr, "Can't find \"scripts\" field in %s\n", path.string().c_str());
result->exit_code_ = ExitCode::kGenericUserError;
return;
}

// If the command_id is not found in the scripts object, throw an error.
std::string_view command;
if (auto command_error =
scripts_object[command_id].get_string().get(command)) {
if (command_error == simdjson::error_code::INCORRECT_TYPE) {
bool have_scripts = main_object["scripts"].get_object().get(scripts_object) ==
simdjson::error_code::SUCCESS;

std::string exec_cmd;
if (have_scripts) {
std::string_view cmd_string;
auto err = scripts_object[command_id].get_string().get(cmd_string);
if (err == simdjson::error_code::SUCCESS) {
exec_cmd.assign(cmd_string);
ProcessRunner runner(result,
path,
command_id,
exec_cmd,
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need this string? You can just pass exec_cmd here?

path_env_var,
positional_args);
runner.Run();
return;
}
if (err == simdjson::error_code::INCORRECT_TYPE) {
fprintf(stderr,
"Script \"%.*s\" is unexpectedly not a string for %s\n\n",
static_cast<int>(command_id.size()),
command_id.data(),
path.string().c_str());
} else {
fprintf(stderr,
"Missing script: \"%.*s\" for %s\n\n",
static_cast<int>(command_id.size()),
command_id.data(),
path.string().c_str());
fprintf(stderr, "Available scripts are:\n");

// Reset the object to iterate over it again
scripts_object.reset();
simdjson::ondemand::value value;
for (auto field : scripts_object) {
std::string_view key_str;
std::string_view value_str;
if (!field.unescaped_key().get(key_str) && !field.value().get(value) &&
!value.get_string().get(value_str)) {
result->exit_code_ = ExitCode::kGenericUserError;
return;
}
}

// Try "bin"
Copy link
Member

Choose a reason for hiding this comment

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

Can you be more explicit? What are we trying?

simdjson::ondemand::value bin_value;
bool have_bin =
main_object["bin"].get(bin_value) == simdjson::error_code::SUCCESS;

if (!have_scripts && !have_bin) {
fprintf(stderr,
"Can't find \"scripts\" or \"bin\" fields in %s\n",
path.string().c_str());
result->exit_code_ = ExitCode::kGenericUserError;
return;
}

if (have_bin) {
simdjson::ondemand::json_type bin_type;
if (!bin_value.type().get(bin_type)) {
std::string exec_rel;

if (bin_type == simdjson::ondemand::json_type::string) {
// "bin": "./cli.js"
std::string_view rel;
if (!bin_value.get_string().get(rel)) {
std::string_view pkg_name;
if (!main_object["name"].get_string().get(pkg_name) &&
pkg_name == command_id) {
exec_rel.assign(rel);
Copy link
Member

Choose a reason for hiding this comment

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

I think you're confusing std::string and std::string_view. These lines are not needed.

} else {
fprintf(stderr, "Incorrect command for %s\n", path.string().c_str());
result->exit_code_ = ExitCode::kGenericUserError;
return;
}
}
} else if (bin_type == simdjson::ondemand::json_type::object) {
// "bin": { "foo": "./cli.js" }
simdjson::ondemand::object bin_obj;
if (bin_value.get_object().get(bin_obj) ==
simdjson::error_code::SUCCESS) {
std::string_view rel;
auto err = bin_obj[command_id].get_string().get(rel);
if (err == simdjson::error_code::SUCCESS) {
exec_rel.assign(rel);
} else if (err == simdjson::error_code::INCORRECT_TYPE) {
fprintf(stderr,
"Bin \"%.*s\" is unexpectedly not a string for %s\n",
static_cast<int>(command_id.size()),
command_id.data(),
path.string().c_str());
result->exit_code_ = ExitCode::kGenericUserError;
return;
}
}
} else {
fprintf(stderr,
"Bin \"%.*s\" is unexpectedly not a string for %s\n",
static_cast<int>(command_id.size()),
command_id.data(),
path.string().c_str());
result->exit_code_ = ExitCode::kGenericUserError;
return;
}

if (!exec_rel.empty()) {
std::filesystem::path exec_path(exec_rel);
if (exec_path.is_relative()) exec_path = path.parent_path() / exec_path;

auto ext = exec_path.extension().string();
bool needs_node = ext == ".js" || ext == ".mjs" || ext == ".cjs";

exec_cmd =
needs_node ? "node " + EscapeShell(exec_path.string()) : exec_rel;

ProcessRunner runner(
result, path, command_id, exec_cmd, path_env_var, positional_args);
runner.Run();
return;
}
}
}

fprintf(stderr,
"Unknown script or bin entry \"%.*s\" for %s\n\n",
static_cast<int>(command_id.size()),
command_id.data(),
path.string().c_str());

if (have_scripts) {
fprintf(stderr, "Available scripts:\n");
scripts_object.reset();
simdjson::ondemand::value value;
for (auto field : scripts_object) {
std::string_view key_str, value_str;
if (!field.unescaped_key().get(key_str) && !field.value().get(value) &&
!value.get_string().get(value_str)) {
fprintf(stderr,
" %.*s: %.*s\n",
static_cast<int>(key_str.size()),
key_str.data(),
static_cast<int>(value_str.size()),
value_str.data());
}
}
} else {
fprintf(stderr, "No scripts defined in %s\n", path.string().c_str());
}

if (have_bin) {
fprintf(stderr, "\nAvailable bins:\n");
simdjson::ondemand::json_type t;
if (!bin_value.type().get(t)) {
if (t == simdjson::ondemand::json_type::string) {
std::string_view rel, pkg_name;
if (!bin_value.get_string().get(rel) &&
!main_object["name"].get_string().get(pkg_name)) {
fprintf(stderr,
" %.*s: %.*s\n",
static_cast<int>(key_str.size()),
key_str.data(),
static_cast<int>(value_str.size()),
value_str.data());
static_cast<int>(pkg_name.size()),
pkg_name.data(),
static_cast<int>(rel.size()),
rel.data());
}
} else if (t == simdjson::ondemand::json_type::object) {
simdjson::ondemand::object bin_obj;
if (!bin_value.get_object().get(bin_obj)) {
bin_obj.reset();
for (auto field : bin_obj) {
std::string_view key_str, rel;
if (!field.unescaped_key().get(key_str) &&
!field.value().get_string().get(rel)) {
fprintf(stderr,
" %.*s: %.*s\n",
static_cast<int>(key_str.size()),
key_str.data(),
static_cast<int>(rel.size()),
rel.data());
}
}
}
}
}
result->exit_code_ = ExitCode::kGenericUserError;
return;
} else {
fprintf(stderr, "No bins defined in %s\n", path.string().c_str());
}

auto runner = ProcessRunner(
result, path, command_id, command, path_env_var, positional_args);
runner.Run();
result->exit_code_ = ExitCode::kGenericUserError;
}

// GetPositionalArgs returns the positional arguments from the command line.
Expand Down
4 changes: 4 additions & 0 deletions test/fixtures/run-script/bin-string/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "bin-test",
"bin": "../test.js"
}
5 changes: 5 additions & 0 deletions test/fixtures/run-script/invalid-bin-value/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"bin": {
"invalid-bin": 2
}
}
1 change: 1 addition & 0 deletions test/fixtures/run-script/invalid-schema/package.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
{
"bin": 1,
"scripts": {
"array": [],
"boolean": true,
Expand Down
6 changes: 6 additions & 0 deletions test/fixtures/run-script/package.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
{
"bin": {
"bin-test": "./test.js",
"test": "./test.js",
"bin-ada": "ada",
"bin-ada-windows": "ada.bat"
},
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1",
"ada": "ada",
Expand Down
3 changes: 3 additions & 0 deletions test/fixtures/run-script/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#!/usr/bin/env node

console.log('bin-test script');
10 changes: 8 additions & 2 deletions test/message/node_run_non_existent.out
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Missing script: "non-existent-command" for *
Unknown script or bin entry "non-existent-command" for *

Available scripts are:
Available scripts:
test: echo "Error: no test specified" && exit 1
ada: ada
ada-windows: ada.bat
Expand All @@ -14,3 +14,9 @@ Available scripts are:
special-env-variables-windows: special-env-variables.bat
pwd: pwd
pwd-windows: cd

Available bins:
bin-test: ./test.js
test: ./test.js
bin-ada: ada
bin-ada-windows: ada.bat
73 changes: 70 additions & 3 deletions test/parallel/test-node-run.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ const assert = require('node:assert');
const fixtures = require('../common/fixtures');
const envSuffix = common.isWindows ? '-windows' : '';

const path = require('node:path');
const nodeDir = path.dirname(process.execPath);
const env = { ...process.env, PATH: `${nodeDir}${path.delimiter}${process.env.PATH}` };

describe('node --run [command]', () => {
it('returns error on non-existent file', async () => {
const child = await common.spawnPromisified(
Expand All @@ -25,6 +29,7 @@ describe('node --run [command]', () => {

it('runs a valid command', async () => {
// Run a script that just log `no test specified`
// Scripts take precedence over bins
const child = await common.spawnPromisified(
process.execPath,
[ '--run', 'test', '--no-warnings'],
Expand Down Expand Up @@ -212,14 +217,76 @@ describe('node --run [command]', () => {
assert.strictEqual(child.code, 1);
});

it('returns error when there is no "scripts" field file', async () => {
it('returns error when there is no "scripts" and "bin" fields in file', async () => {
const child = await common.spawnPromisified(
process.execPath,
[ '--run', 'test'],
{ cwd: fixtures.path('run-script/cannot-find-script') },
{ cwd: fixtures.path('run-script/cannot-find-script-and-bin') },
);
assert.match(child.stderr, /Can't find "scripts" field in/);
assert.match(child.stderr, /Can't find "scripts" or "bin" fields in/);
assert.strictEqual(child.stdout, '');
assert.strictEqual(child.code, 1);
});

it('print avilables scripts and bins when command not found', async () => {
const child = await common.spawnPromisified(
process.execPath,
[ '--run', 'tmp'],
{ cwd: fixtures.path('run-script') },
);
assert.match(child.stderr, /Unknown script or bin entry "tmp"/);
assert.match(child.stderr, /Available scripts:\n/);
assert.match(child.stderr, /ada: ada\n/);
assert.match(child.stderr, /Available bins:\n/);
assert.match(child.stderr, /bin-test: \.\/test\.js\n/);
Comment on lines +237 to +241
Copy link
Member

Choose a reason for hiding this comment

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

Can you convert this into a snapshot test? It's extremely hard to understand this assertions.

});

describe('Bin scripts use cases', () => {
it('runs a bin from package.json object format', async () => {
const child = await common.spawnPromisified(
process.execPath,
['--run', 'bin-test'],
{ cwd: fixtures.path('run-script'), env },
);
assert.match(child.stdout, /bin-test script/);
});

it('handles error with invalid bin value in object format', async () => {
const child = await common.spawnPromisified(
process.execPath,
['--run', 'invalid-bin'],
{ cwd: fixtures.path('run-script/invalid-bin-value'), env },
);
assert.match(child.stderr, /Bin "invalid-bin" is unexpectedly not a string/);
});

it('runs a bin from package.json string format', async () => {
const child = await common.spawnPromisified(
process.execPath,
['--run', 'bin-test'],
{ cwd: fixtures.path('run-script/bin-string'), env },
);
assert.match(child.stdout, /bin-test script/);
});

it('handles error with invalid bin value in string format', async () => {
const child = await common.spawnPromisified(
process.execPath,
['--run', 'invalid-bin'],
{ cwd: fixtures.path('run-script/invalid-schema'), env }
);
assert.match(child.stderr, /Bin "invalid-bin" is unexpectedly not a string/);
});

it('adds node_modules/.bin to path', async () => {
const child = await common.spawnPromisified(
process.execPath,
['--run', `bin-ada${envSuffix}`],
{ cwd: fixtures.path('run-script') },
);
assert.match(child.stdout, /06062023/);
assert.strictEqual(child.stderr, '');
assert.strictEqual(child.code, 0);
});
});
});