Fix Node 24-specific deprecation warning#90
Conversation
| "ava": "^6.1.3", | ||
| "c8": "^10.1.2", | ||
| "get-node": "^15.0.1", | ||
| "log-process-errors": "^12.0.1", |
There was a problem hiding this comment.
Deprecation warnings do not make tests fail, which is why we missed it. This dev dependency will ensure tests fail instead.
It also shows more verbose logs when that happens.
Before:
✔ options › options.env augments process.env
(node:23634) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true can lead to security vulnerabilities, as the arguments are not escaped, only concatenated.
(Use `node --trace-deprecation ...` to show where the warning was created)
✔ options › options.preferLocal undefined does not run local npm binaries (209ms)
After:
Unhandled rejection in test/options.js
test/options.js:41
40: const testArgv0 = async (t, shell) => {
41: const {stdout} = await spawn(...nodePrintArgv0, {argv0: testString, shell});
42: t.is(stdout, shell ? process.execPath : testString);
DeprecationWarning: Passing args to a child process with shell option true can lead to security vulnerabilities, as the arguments are not escaped, only concatenated.
This promise was rejected but not handled.
› This promise was rejected but not handled.
› spawnSubprocess (file://source/spawn.js:17:20)
› async getResult (file://source/result.js:5:19)
› async testArgv0 (file://test/options.js:41:19)
✘ test/options.js exited with a non-zero exit code: 1
Disclaimer: this is my own library but it's been around for 7 years, and this is only added as a dev dependency.
| // So we do want users to pass array of arguments even with `shell: true`, but we also want to avoid any warning. | ||
| const concatenateShell = (file, commandArguments, options) => options.shell && commandArguments.length > 0 | ||
| ? [[file, ...commandArguments].join(' '), [], options] | ||
| : [file, commandArguments, options]; |
There was a problem hiding this comment.
From the Node.js source code, this should be doing exactly what Node.js is doing. So this is not breaking, and should have no changes except for not showing the deprecation warning anymore. So this can be a patch release.
Also, when Node.js does remove support altogether, this won't impact us anymore.
👍 |
Fixes #89.
After merging this PR, I am going to create a similar one for Execa, if that sounds with you?