-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
cli: implement node --run <script-in-package-json>
#52190
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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'); | ||||||||||||||||||||||
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(); | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: jsonString (JS convesions and not C++/Python) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||
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 }); | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was re-written in cpp 🙂 Lines 286 to 288 in b1c1faf
Lines 216 to 222 in b1c1faf
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, @atlowChemi for clarifying! |
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, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -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 | ||
|
@@ -344,6 +360,28 @@ void BindingData::GetNearestParentPackageJSONType( | |
args.GetReturnValue().Set(Array::New(realm->isolate(), values, 3)); | ||
} | ||
|
||
void BindingData::GetPackageJSONScripts( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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".
I'm not blocking of that but the approach here seems like the worst of both worlds? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll follow up and rewrite it in C++ There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -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, | ||
|
@@ -440,6 +479,7 @@ void BindingData::RegisterExternalReferences( | |
registry->Register(GetNearestParentPackageJSONType); | ||
registry->Register(GetNearestParentPackageJSON); | ||
registry->Register(GetPackageScopeConfig); | ||
registry->Register(GetPackageJSONScripts); | ||
} | ||
|
||
} // namespace modules | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
CUSTOM_ENV="hello world" |
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.
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.
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.
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" | ||
} | ||
} |
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); |
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 |
Uh oh!
There was an error while loading. Please reload this page.