Skip to content

Conversation

@sorccu
Copy link
Member

@sorccu sorccu commented Feb 20, 2025

Replaces removed ux.wait() with stdlib, ux.prompt() with prompts.

Minor changes to bin files were also required as otherwise errors were printed with stack traces. I had not realized that the create-cli bin was checking Node version - it has now been updated to Node 18 too. The new oclif also requires Node 18.

The default command which was used for the create-cli no longer exists and was replaced with the single command strategy.

Custom help output had some type changes but is basically the same.

Note: for whatever reason we now MUST set the oclif topicSeparator. This makes no sense but if topicSeparator is not present or is set to ":", CLI errors such as attempting to run bin/run foo in the packages/create-cli folder will error with a stack trace, with the error being thrown and handled (incorrectly) by oclif itself.

I hereby confirm that I followed the code guidelines found at engineering guidelines

Affected Components

  • CLI
  • Create CLI
  • Test
  • Docs
  • Examples
  • Other

Notes for the Reviewer

Resolves #[issue-number]

New Dependency Submission

@sorccu sorccu requested a review from tnolet February 20, 2025 19:03
@tnolet
Copy link
Member

tnolet commented Feb 21, 2025

@sorccu tests are still failing. I will hold off on review till they pass.

@sorccu sorccu force-pushed the simokinnunen/sc-23314/cli-update-oclif branch from 955bf6a to 827b222 Compare February 25, 2025 11:13
@tnolet
Copy link
Member

tnolet commented Feb 25, 2025

@sorccu I actually would vote to introduce full stack traces. I find it very annoying to debug our current CLI as it does not print usable runtime exceptions due to the missing stack traces. Maybe we can switch it based on a DEBUG env variable?

Replaces removed ux.wait() with stdlib, ux.prompt() with prompts.

Minor changes to bin files were also required as otherwise errors were
printed with stack traces. I had not realized that the create-cli bin
was checking Node version - it has now been updated to Node 18 too. The new
oclif also requires Node 18.

Custom help output had some type changes but is basically the same.
@sorccu sorccu force-pushed the simokinnunen/sc-23314/cli-update-oclif branch from 827b222 to 67b2a30 Compare February 25, 2025 16:26
…erly

This makes no sense but if topicSeparator is not present or is set to `":"`,
CLI errors such as attempting to run `bin/run foo` in the packages/create-cli
folder will error with a stack trace, with the error being thrown and handled
(incorrectly) by oclif itself.
Copy link
Member

@tnolet tnolet left a comment

Choose a reason for hiding this comment

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

Smoke tested locally. LGTM

@sorccu sorccu merged commit 45ca172 into main Feb 26, 2025
3 checks passed
@sorccu sorccu deleted the simokinnunen/sc-23314/cli-update-oclif branch February 26, 2025 13:19
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.

2 participants