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

Upgrade commander dependency #2613

Closed
maschad opened this issue Jun 24, 2024 · 8 comments · Fixed by #2620
Closed

Upgrade commander dependency #2613

maschad opened this issue Jun 24, 2024 · 8 comments · Fixed by #2620
Assignees
Labels
chore Issue is a chore good first issue Suitable for newcomers looking to contribute

Comments

@maschad
Copy link
Contributor

maschad commented Jun 24, 2024

Dependabot is unable to upgrade this FuelLabs/fuels-ts/actions/runs/9649869028/job/26614245232?pr=2599 so it needs to be done manually.

This dependency should be upgraded to latest version and all CI tests should pass.

@maschad maschad added chore Issue is a chore p2 good first issue Suitable for newcomers looking to contribute labels Jun 24, 2024
@maschad maschad added this to the 0.x post-launch milestone Jun 24, 2024
@YaTut1901
Copy link
Contributor

hello, I can try it

@petertonysmith94
Copy link
Contributor

hello, I can try it

@YaTut1901 Go for it! If you need any help, don't hesitate to ask 😄

@YaTut1901
Copy link
Contributor

@petertonysmith94, I am running tests with bumped dependency and they are failing because of commands clashing. Here it is, in file packages/fuels/src/cli.ts:

45: const pathOption = new Option('-p, --path <path>', 'Path to project root').default(process.cwd());
58: .addOption(new Option(`-p, --predicates ${arg}`, `${desc} Predicates`).conflicts('workspace'))

updated commander doesn`t want to accept same options "-p", should they be renamed?

@petertonysmith94
Copy link
Contributor

@petertonysmith94, I am running tests with bumped dependency and they are failing because of commands clashing. Here it is, in file packages/fuels/src/cli.ts:

45: const pathOption = new Option('-p, --path <path>', 'Path to project root').default(process.cwd());
58: .addOption(new Option(`-p, --predicates ${arg}`, `${desc} Predicates`).conflicts('workspace'))

updated commander doesn't want to accept same options "-p", should they be renamed?

@YaTut1901 Great catch!

Not sure how this hasn't been an issue before, as they are conflicting flags. I'd suggest changing the path option to be capital P, as I would guess this would be a less breaking change to end users.

const pathOption = new Option('-P, --path <path>', 'Path to project root').default(process.cwd());

@arboleya
Copy link
Member

arboleya commented Jun 25, 2024

Oh wow, how come did this get this far? 🤦‍♂️

@petertonysmith94 I'd say we don't need a shortcut for path, so it can be just --path. In my experience, the --path option is not very used, and having a case where we'd have -P and -p simultaneously may look weird. It works, but we can avoid it and favor clarity over brevity.

It is good to note that this will be a [small] breaking change.

@YaTut1901
Copy link
Contributor

YaTut1901 commented Jun 25, 2024

by the way, when I run tests on latest master, I get the same errors
And some other errors because of incorrect usage of Command and program from commander:
packages/fuels/src/cli/commands/withBinaryPaths.test.ts:

1 import { program } from 'commander';
15 const command = program
16     .command(Commands.versions)

It is not creating new command, but rather use the global one. That leads to such error messages:
Error: cannot add command 'versions' as already have command 'versions'
For me seems better way is:

1 import { Command } from 'commander';
15 const command = new Command()
16     .command(Commands.versions)

@YaTut1901
Copy link
Contributor

@petertonysmith94 is there any other issues need to be completed?

@petertonysmith94
Copy link
Contributor

@YaTut1901 Check out the good first issues - just pop a message on the issue :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Issue is a chore good first issue Suitable for newcomers looking to contribute
Projects
None yet
4 participants