Skip to content

feat: Support required arguments in CLI commands #2658

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

robhogan
Copy link
Collaborator

@robhogan robhogan commented May 7, 2025

Summary

While doing some work on react-native bundle I noticed some arguments we require are actually not requiredOptions in Commander, and I was surprised that there was no way to mark an option required in config.

This adds the required field to CommandOption and passes it through so that we can more easily validate and emit user-actionable errors.

Test Plan

Made these changes locally (with link) against RN 0.80-rc.0, and tested by adding required to the bundle command:

Before

  • Fails with obscure error on path.dirname(undefined)
yarn run react-native bundle --entry-file index.js --verbose --platform ios --dev
yarn run v1.22.22
$ /Users/robhogan/workspace/ReactNative080rc0/node_modules/.bin/react-native bundle --entry-file index.js --verbose --platform ios --dev
                Welcome to Metro v0.82.3
              Fast - Scalable - Integrated


error The "path" argument must be of type string. Received undefined.
TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received undefined
    at Object.dirname (node:path:1371:5)
    at buildBundleWithConfig (/Users/robhogan/workspace/ReactNative080rc0/node_modules/@react-native/community-cli-plugin/dist/commands/bundle/buildBundle.js:73:44)
    at async Command.handleAction (/Users/robhogan/workspace/ReactNative080rc0/node_modules/@react-native-community/cli/build/index.js:139:9)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

After

  • Fails fast with actionable error
yarn run react-native bundle --entry-file index.js --verbose --platform ios --dev
yarn run v1.22.22
$ /Users/robhogan/workspace/ReactNative080rc0/node_modules/.bin/react-native bundle --entry-file index.js --verbose --platform ios --dev
error: required option '--bundle-output <string>' not specified
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Checklist

  • Documentation is up to date.
  • Follows commit message convention described in CONTRIBUTING.md.
  • For functional changes, my test plan has linked these CLI changes into a local react-native checkout (instructions).

NOTE: e2e tests are hanging for me so I wasn't able to validate the new tests

@github-actions github-actions bot added docs Documentation change feature labels May 7, 2025
'--optional-arg',
'foo',
]);
expect(stdout).toBe('test-command-ts');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These tests are broken - working on it - don't merge ;)

@robhogan robhogan force-pushed the robhogan/required-args-in-cmds branch 2 times, most recently from 7210bd5 to 250b936 Compare May 7, 2025 16:11
@robhogan robhogan force-pushed the robhogan/required-args-in-cmds branch from 250b936 to 4c192ab Compare May 7, 2025 17:36
@robhogan
Copy link
Collaborator Author

robhogan commented May 7, 2025

The new test is passing for me locally now, there's a pre-existing break with the "shows up current config" test.
image

I've no idea what's going on with GHA CI tbh

@thymikee
Copy link
Member

thymikee commented May 7, 2025

We need to rethink these e2e tests. Currently they’re installing whole new project and cocoa pods for each case which takes too much time. I’m good with marking some as test.todo because this CI is red for too long

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

Successfully merging this pull request may close these issues.

2 participants