Skip to content
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

fix: node version check now uses process.versions.node #450

Merged
merged 7 commits into from
Aug 3, 2022
Merged

fix: node version check now uses process.versions.node #450

merged 7 commits into from
Aug 3, 2022

Conversation

paperdave
Copy link
Contributor

Closes #447 by using process.versions.node for testing the version. The only difference between these values is the v prefix, so it's not as trivial as swapping a variable. Also, just in case process.versions is somehow not set, this falls back to previous behavior.

With this change in place, Bun should be able to import yargs.

@paperdave paperdave changed the title fix: node version uses process.versions.node fix: node version check now uses process.versions.node Jul 13, 2022
Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix seem reasonable to me, left a small nit.

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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you do:

const nodeVersion = process?.versions?.node && process.version.slice(1);

To simplify the logic slightly?

Copy link
Contributor Author

@paperdave paperdave Jul 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lib/index.ts Outdated Show resolved Hide resolved
@bcoe
Copy link
Member

bcoe commented Jul 19, 2022

@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.

@paperdave
Copy link
Contributor Author

weird. i couldnt reproduce the failing test on my machine but i nonetheless updated the config.

@paperdave paperdave requested a review from bcoe July 21, 2022 04:10
@bcoe bcoe merged commit d07bcdb into yargs:main Aug 3, 2022
@bcoe
Copy link
Member

bcoe commented Aug 3, 2022

@paperdave thank you for the contribution 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version number check fails on Bun
2 participants