cli: add support to bin scripts #58172
Conversation
nodejs-github-bot
added
c++
IlyasShabi
changed the title
IlyasShabi
force-pushed
the
ishabi/bin-scripts
branch
from
e8fd4dc to
959e66f
Compare
|
I'm -1 on this. Node.js already bundles npm, which has built-in support for this. I'm not 100% sure, but surely other package managers supports it as well. This is a feature that simply works out of the box. Refs: |
There was a problem hiding this comment.
This change needs a documentation change, before others can review. I have almost no idea what this pull-request is, and had to lookup package.json spec to understand. The code should be self-explanatory. If the solution requires complexity, than we need comments everywhere
| 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/); |
There was a problem hiding this comment.
Can you convert this into a snapshot test? It's extremely hard to understand this assertions.
| ProcessRunner runner(result, | ||
| path, | ||
| command_id, | ||
| exec_cmd, |
There was a problem hiding this comment.
Why do you need this string? You can just pass exec_cmd here?
| @@ -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. | |||
| } | ||
| } | ||
|
|
||
| // Try "bin" |
There was a problem hiding this comment.
Can you be more explicit? What are we trying?
| std::string_view pkg_name; | ||
| if (!main_object["name"].get_string().get(pkg_name) && | ||
| pkg_name == command_id) { | ||
| exec_rel.assign(rel); |
There was a problem hiding this comment.
I think you're confusing std::string and std::string_view. These lines are not needed.
Description
This PR makes
node --run <cmd>run commands from the package’s bin section as well. If the command is in both scripts and bin, the version in scripts still wins.#53845
TODO