-
Notifications
You must be signed in to change notification settings - Fork 21
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
Bump which
to v4
#139
Bump which
to v4
#139
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
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.
The changes here look good. Although which dropped support for Node 14, CI still runs with 14. If the tests are green, I suppose that we are good to go.
@@ -12,6 +12,10 @@ var sandbox = require('sandboxed-module'); | |||
var binary = utils.normalizeBinary; | |||
|
|||
sandbox.configure({ | |||
requires: { | |||
// sandboxed-module is unable to find this module directly |
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.
... because it is not listed at https://github.com/felixge/node-sandboxed-module/blob/v2.0.4/lib/builtin_modules.json and would therefore result in the module being resolved as a relative file path?
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 opened a PR upstream. This PR can be merged in the meanwhile because that package hasn't seen updates in 4 years.
@@ -23,7 +23,7 @@ | |||
"dependencies": { | |||
"commander": "2.9.0", | |||
"shell-quote": "1.8.1", | |||
"which": "1.2.4", | |||
"which": "^4.0.0", |
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.
Version 4.0.0 drops Node 14 support, and does not have other significant changes, per https://github.com/npm/node-which/blob/main/CHANGELOG.md#400-2023-08-29
If we are planning to release a major version then this lgtm. Otherwise we should use 3.0.1 instead.
Considering that we are indeed preparing for a major version in #89, 4.0.0 here is acceptable.
In retrospect, I should have updated
which
before or together with droppingwhen
😅 but hindsight is 20/20