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

Replace commander with parseArgs #56

Open
RaisinTen opened this issue Oct 20, 2022 · 8 comments · May be fixed by #67
Open

Replace commander with parseArgs #56

RaisinTen opened this issue Oct 20, 2022 · 8 comments · May be fixed by #67
Labels
cli 🖥️ enhancement ✨ New feature or request good first issue Good for newcomers

Comments

@RaisinTen
Copy link
Contributor

See nodejs/node#45038 (comment)

@dsanders11
Copy link
Contributor

I don't think parseArgs is a replacement for commander in Postject, it's too minimal. For example, we'd lose the whole help functionality and usage output we currently have. It would also make #43 more trickier, while commander provides sub-command functionality.

@RaisinTen
Copy link
Contributor Author

Yea that makes sense to me. @ruyadorno do you have any other suggestions on this?

@ruyadorno
Copy link
Member

thanks @RaisinTen for bringing this up!

@dsanders11 I agree 100% with your initial statement, the goal of the native parseArgs module was never to replace full blown cli frameworks such as commander.

That said, my proposal to use it here instead is based on different types of motivations:

  1. Given the intention to vendor postject within Node.js, it would be great to use its native solution and avoid adding an extra bundled dependency
  2. This can be a great opportunity to set good examples/standards on how to use parseArgs on a real life cli application
  3. By moving postject to the Node.js org, more contributors will be attracted to the project, which can help ease the extra work load, also these contributors might be more familiar with the standard parseArgs module than a specific cli framework (myself included)

Outside of these practical reasons, there are also the relevance of helping creating more synergy between the collaborators of postject and the larger pool of Node.js collaborators. I'd encourage the team here to report and even propose improvements to the parseArgs module moving forward. Also relevant to this point, did you know parseArgs is the result of a lot of collaboration between the maintainers of both commander and yargs that originally took place at https://github.com/pkgjs/parseargs as part of a Node.js Tooling Group initiative? 😊 A big motivation of my initial idea was helping promote this type of collaborative work!

With all that in mind I totally respect your call on this. Just let me know in case you want to move forward with parseArgs since I might be able to contribute a little to the this specific initiative and also help rally more folks from the Working Groups that might be willing to help with the work necessary.

@jviotti
Copy link
Member

jviotti commented Oct 21, 2022

As a big fan of reducing dependency scope, I'm +1 on the idea of ditching commander if it's doable.

@RaisinTen
Copy link
Contributor Author

RaisinTen commented Oct 22, 2022

@ruyadorno thanks for chiming in and also thank you for the great points!

Given the intention to vendor postject within Node.js, it would be great to use its native solution and avoid adding an extra bundled dependency

The way I initially imagined node would use postject is through the programmatic API here - https://github.com/postmanlabs/postject/blob/b9356bf75ba4c22aab6baeaab51a94111ef4a79b/src/api.js#L6, i.e., the inject() function. This file does not do require('commander'), so commander would not be a part of the bundled JS file that was being used inside node.

However, I agree with the rest of the points you raised and I'm fully supportive of using parseArgs if possible without adding a lot of complexity. :-)

@bakkot
Copy link

bakkot commented Dec 2, 2022

It would also make #43 more trickier, while commander provides sub-command functionality.

Incidentally if you do go for this, subcommands are doable in parseArgs with a relatively small amount of boilerplate. More work than commander, but not that bad.

@tony-go
Copy link
Member

tony-go commented Dec 2, 2022

+1 for using native solution

@sheplu
Copy link
Member

sheplu commented Dec 2, 2022

+1 for using parseArgs

debadree25 added a commit to debadree25/postject that referenced this issue Dec 18, 2022
@debadree25 debadree25 linked a pull request Dec 18, 2022 that will close this issue
@RaisinTen RaisinTen linked a pull request Dec 19, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli 🖥️ enhancement ✨ New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants