fix: node version check now uses process.versions.node #450
Conversation
paperdave
changed the title
lib/index.ts
Outdated
| @@ -19,8 +19,9 @@ import { readFileSync } from 'fs' | |||
| const minNodeVersion = (process && process.env && process.env.YARGS_MIN_NODE_VERSION) | |||
| ? Number(process.env.YARGS_MIN_NODE_VERSION) | |||
| : 12 | |||
| if (process && process.version) { | |||
| const major = Number(process.version.match(/v([^.]+)/)![1]) | |||
| const nodeVersion = process && (process.versions ? process.versions.node : process.version.slice(1)) | |||
There was a problem hiding this comment.
Could you do:
const nodeVersion = process?.versions?.node && process.version.slice(1);
To simplify the logic slightly?
There was a problem hiding this comment.
this should be an || in your snippet, but yes the logic could be simplified. i wasn't sure if i was able to use ?. in the code.
There was a problem hiding this comment.
i have edited this, but do note that this syntax doesn't work below node v14, though your build process makes sure this gets transpiled down. it should be noted that this syntax isnt used anywhere else in the project yet.
|
@davecaruso I believe there's been a small dip in coverage because of the newline added, please feel free to update the file here: https://github.com/yargs/yargs-parser/blob/main/.nycrc On your branch, to bring the threshold down to 97. |
|
weird. i couldnt reproduce the failing test on my machine but i nonetheless updated the config. |
|
@paperdave thank you for the contribution 👌 |
Closes #447 by using
process.versions.nodefor testing the version. The only difference between these values is thevprefix, so it's not as trivial as swapping a variable. Also, just in caseprocess.versionsis somehow not set, this falls back to previous behavior.With this change in place, Bun should be able to import yargs.