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

Compatibility with node and yarn: process.version #420

Closed
coderaiser opened this issue Jul 8, 2022 · 13 comments
Closed

Compatibility with node and yarn: process.version #420

coderaiser opened this issue Jul 8, 2022 · 13 comments
Labels
bug Something isn't working node.js Compatibility with Node.js APIs

Comments

@coderaiser
Copy link

coderaiser commented Jul 8, 2022

1008 |     ? Number(process.env.YARGS_MIN_NODE_VERSION)
1009 |     : 12;
1010 | if (process1008 |     ? Number(process.env.YARGS_MIN_NODE_VERSION)
1009 |     : 12;
1010 | if (process && process.version) {
1011 |     const major = Number(process.version.match(/v([^.]+)/)[1]);
1012 |     if (major < minNodeVersion) {
1013 |         throw Error(`yargs parser supports a minimum Node.js version of ${minNodeVersion}. Read our version support policy: https://github.com/yargs/yargs-parser#supported-nodejs-versions`);
                  ^
 error: yargs parser supports a minimum Node.js version of 12. Read our version support policy: https://github.com/yargs/yargs-parser#supported-nodejs-versions
      at /Users/coderaiser/putout/packages/putout/node_modules/yargs-parser/build/index.cjs:1013:14
      at bun:wrap:1:16430
      at /Users/coderaiser/putout/packages/putout/lib/cli/index.js:5:28
      at bun:wrap:1:16430
      at /Users/coderaiser/putout/packages/putout/bin/putout.mjs:10:0&& process.version) {

Setting YARGS_MIN_NODE_VERSION=0.1.2 solves the issue, but then I see:

error: Could not resolve: "readline". Maybe you need to "bun install"?

const readline = require('readline');

Without a trailing newline.

Also a little question: is it possible to pass command line arguments to script I run with bun?

bun bin/putout.mjs -f dump

-f dump is handled by bun, but I need it to use other formatter that don't uses readline :).

@FinnRG
Copy link
Collaborator

FinnRG commented Jul 8, 2022

  1. The readline issue is documented in readline not available #311
  2. You can pass CLI arguments by writing bun bin/putout.mjs -- -f dump

@FinnRG
Copy link
Collaborator

FinnRG commented Jul 8, 2022

The version issue was also resolved by #392. Please close this issue.

@coderaiser
Copy link
Author

Thanks for a quick response!

@paperdave
Copy link
Collaborator

The version issue was also resolved by #392. Please close this issue.

this only modifies the napi function, and not process.version. broken on bun 0.1.3 still.

@coderaiser coderaiser reopened this Jul 12, 2022
@FinnRG
Copy link
Collaborator

FinnRG commented Jul 12, 2022

@davecaruso You're right, I'm sorry about that. Since we probably don't want to change our current process.version output, we could expose a flag like --set-version which replaces every occurrence of process.version with the set version (similar to how we replace env variable).

@paperdave
Copy link
Collaborator

we could expose a flag like --set-version which replaces every occurrence of process.version with the set version (similar to how we replace env variable).

I dislike this because it now requires anyone using yargs or depending on a package that uses it, all devs and users need that flag. It feels the same as YARGS_MIN_NODE_VERSION, and env variable you have to specify everywhere. that would still be better than nothing, but a lot of people will try this (and any other package that may do this type of check) and complain it doesn't work.

but i do think process.version should contain the bun one since it's bun that we're running and not node. maybe they can convince yargs to switch to reading process.versions.node, but i kind of feel like they won't implement that change.

@FinnRG
Copy link
Collaborator

FinnRG commented Jul 12, 2022

@davecaruso We should definitely try to get this into yargs. As a temporary workaround, you can use this flag: --define process.version:"\"13.0.0\"", which replaces every occurrence of process.version with the specified version.

@coderaiser
Copy link
Author

I think the best possible process.version would be Nodejs LTS that Bun going to be compatible with and for Bun version it can be process.bunVersion or something like this.

@FinnRG
Copy link
Collaborator

FinnRG commented Jul 13, 2022

Good news: The yargs-parser team seems to be open about adding feature detection for bun: yargs/yargs-parser#447 (comment)

@paperdave
Copy link
Collaborator

and better news, i just opened PR yargs/yargs-parser#450 which should fix it. this obviously wont fix any other libraries doing this, but they should all change to this way of checking node.

@xHyroM
Copy link
Collaborator

xHyroM commented Jul 17, 2022

If someone really needs to edit process.version, you'll need two files at best. One will do:

// @ts-expect-error
process.version = 300;

await import("./other-file");

And the other one then imports the modules like yargs, etc. define flag is better

@paperdave
Copy link
Collaborator

btw, the process version check has been fixed, but i believe theres some other stuff we need before yargs fully works on bun; however those steps are our responsibility here and not theirs.

@Electroid Electroid added bug Something isn't working node.js Compatibility with Node.js APIs labels Nov 1, 2022
@Electroid
Copy link
Contributor

process.version is supported as of 0.2.2. For other compatibility issues, please file another issue and we'll track it down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working node.js Compatibility with Node.js APIs
Projects
None yet
Development

No branches or pull requests

5 participants