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(deps): upgrade commander #7008

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from
Draft

fix(deps): upgrade commander #7008

wants to merge 20 commits into from

Conversation

serhalp
Copy link
Contributor

@serhalp serhalp commented Jan 21, 2025

This is currently stacked on #7019.

TODO my solution to the "hidden" global options like --json that were redefined in some commands is resulting in the generated reference docs dropping those options from those commands... We need to either update the script to read from "help options" on the command object (at first glance it doesn't seem to be available...) or try a different approach (like removing these entirely from BaseCommand and defining them as regular options on the individual commands that do want them).

Comment on lines -195 to -199
.addOption(
new Option('--httpProxy [address]', 'Old, prefer --http-proxy. Proxy server address to route requests through.')
.default(process.env.HTTP_PROXY || process.env.HTTPS_PROXY)
.hideHelp(true),
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like there was an httpProxy option and an http-proxy option but these got accidentally(?) squashed into a duplicated httpProxy option in https://github.com/netlify/cli/pull/5816/files#diff-67082469659283bc342bd13f7e20570cb3d1bf5096aad88545a9ac4809d6083bL106-L140. The latest version of commander throws when you declare duplicate options.

What's extra confusing is the copy here implies that http-proxy was intended to be the new one but that's not where it landed 🤷🏼.

@@ -27,6 +27,7 @@ export const detectNetlifyLambda = async function ({ packageJson } = {}) {
.option('-b, --babelrc [file]')
.option('-t, --timeout [delay]')

program.allowExcessArguments()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default behaviour changed in the commander upgrade. This opts back into the previous behaviour. We might consider changing at some point, but it didn't seem right to include this in this PR (and require us to cut a major).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(To be clear here, I ended up only upgrading to commander@12 after all. This behavioural change is in commander@13, but I left this here so we're ready for it, since it doesn't hurt.)

.option('-o, --output <path>', 'Defines the filesystem path where the blob data should be persisted')
.option('-O, --output <path>', 'Defines the filesystem path where the blob data should be persisted')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

-o is already used in the base command for --offline, so this was being quietly ignored (I tested it locally).

This is fine to be considered a patch change since it has never worked.

Caught by changes in commander upgrade: tj/commander.js#2055.

Comment on lines -56 to +57
.option('--json', `Output list contents as JSON`)
// The BaseCommand defines a `--json` option which is hidden from the help by default
.addHelpOption(new Option('--json', 'Output list contents as JSON'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above here, but in this case it's intentional. Just removing this line works, but then --json no longer shows up in ntl blobs:get --help. This solution seems to cover all the bases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... and then same for all the command --offline cases

@serhalp serhalp force-pushed the fix/upgrade-misc-deps branch 3 times, most recently from c3cdea8 to b807744 Compare January 21, 2025 21:30
.option('-o, --open', 'Open site after deploy', false)
.option('-O, --open', 'Open site after deploy', false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here, there's already -o for --offline in the BaseCommand, so deploy -o wasn't working as intended. It just started an offline deploy which of course didn't work.

Copy link

github-actions bot commented Jan 23, 2025

📊 Benchmark results

Comparing with 8a51e34

  • Dependency count: 1,194 ⬆️ 2.01% increase vs. 8a51e34
  • Package size: 307 MB ⬇️ 2.94% decrease vs. 8a51e34
  • Number of ts-expect-error directives: 801 ⬇️ 0.37% decrease vs. 8a51e34

There's some sort of undocumented breaking visual change that's messing with snapshot tests.
It looks like there's some sort of change in inquirer-autocomplete-prompt and it's a can
of worms to troubleshoot. This in turn prevents upgrading inquirer past v8.
It looks like there was an `httpProxy` option and an `http-proxy` option but these got
accidentally(?) squashed into a duplicated `httpProxy` option in
https://github.com/netlify/cli/pull/5816/files#diff-6708246L106-L140.
`-o` is already used in the base command for `--offline`, so this was being quietly ignored

This can't be considered a breaking change as it has never worked. (I just tried it to be sure.)

See tj/commander.js#2055.
It's already used in the BaseCommand for `--offline`.

This is not a breaking change, because this never worked as is.

Same issue as in 9280c55.
It's already defined in the BaseCommand for `--offline`.
- `-r` was already used globally so this was being ignored
- the camelCase version was using the same short option as the kebab-case, which isn't allowed
@serhalp serhalp force-pushed the fix/upgrade-misc-deps branch from 0d11dde to 6aa83f2 Compare February 4, 2025 22:45
@serhalp serhalp changed the title fix(deps): upgrade 23 outdated dependencies fix(deps): upgrade commander Feb 4, 2025
@serhalp serhalp changed the base branch from main to fix/upgrade-misc-deps-except-commander February 4, 2025 22:46
@serhalp serhalp force-pushed the fix/upgrade-misc-deps branch from 1fb55f2 to ad03c5f Compare February 4, 2025 23:11
Base automatically changed from fix/upgrade-misc-deps-except-commander to main February 6, 2025 00:03
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.

1 participant