-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
tools: remove npm install from acorn update #50473
tools: remove npm install from acorn update #50473
Conversation
Review requested:
|
@@ -17,7 +17,7 @@ DEPS_DIR="$BASE_DIR/deps" | |||
. "$BASE_DIR/tools/dep_updaters/utils.sh" | |||
|
|||
NEW_VERSION=$("$NODE" "$NPM" view acorn-walk dist-tags.latest) | |||
CURRENT_VERSION=$("$NODE" -p "require('./deps/acorn/acorn-walk/package.json').version") | |||
CURRENT_VERSION=$("$NODE" "$NPM" --prefix './deps/acorn/acorn-walk/' pkg get version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for this? Using npm seems overkill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a particular reason, it feels more clean since its npm pkg get does exactly that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the original one and it's faster.
|
||
NEW_VERSION=$("$NODE" "$NPM" view acorn dist-tags.latest) | ||
CURRENT_VERSION=$("$NODE" -p "require('./deps/acorn/acorn/package.json').version") | ||
CURRENT_VERSION=$("$NODE" "$NPM" --prefix './deps/acorn/acorn/' pkg get version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
Landed in 2a1bd66 |
PR-URL: nodejs#50473 Refs: nodejs/security-wg#1037 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
PR-URL: #50473 Refs: nodejs/security-wg#1037 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
PR-URL: #50473 Refs: nodejs/security-wg#1037 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
PR-URL: #50473 Refs: nodejs/security-wg#1037 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
Refs: nodejs/security-wg#1037