feat(cli): command line spinner #3968
Conversation
|
|
There was a problem hiding this comment.
I'm unsure about adding this also. It seems like something useful that other modules could rely on. However, it could become too opinionated if we're not careful. Either way, I've added a few suggestions. I'm happy to defer to whatever the concensus is for this addition.
I think the only exported symbols here should be Spinner and SpinnerOptions.
|
@iuioiua Thanks for the feedback, I incorporated everything. Also, do you have any suggestions for how I might add tests for this? |
|
Sorry, I don't see the changes. Did you push your changes? I'm not sure how to test this yet. In this case, we may need to do manual testing for the first pass. |
… createSpinner function
|
I think I fixed it now. |
cli/spinner.ts
Outdated
| * | ||
| * @default {75} | ||
| */ | ||
| speed?: number; |
There was a problem hiding this comment.
speed sounds strange to me as lower number gives faster speed of spinner. How about internal? ref: https://github.com/sindresorhus/ora?tab=readme-ov-file#interval
cli/spinner.ts
Outdated
| */ | ||
| speed?: number; | ||
| /** The color of the spinner. Defaults to the default terminal color. */ | ||
| color?: string; |
There was a problem hiding this comment.
It seems this needs to be ANSI color code. On the other hand, ora supports human-readable color names ('black' | 'red' | 'green' | 'yellow' | 'blue' | 'magenta' | 'cyan' | 'white' | 'gray'). I wonder which is the better design..
There was a problem hiding this comment.
I think it definitely makes it easier to read and write quickly. I'll make it support both 👍
cli/spinner.ts
Outdated
| type Ansi = string & { }; | ||
| type Color = 'black' | 'red' | 'green' | 'yellow' | 'blue' | 'magenta' | 'cyan' | 'white' | 'gray' | Ansi; |
There was a problem hiding this comment.
@iuioiua I used this typescript hack to allow for type suggestions on colors and allowing for a ANSI string, is this ok?
| spinner?: string[]; | ||
| /** The message to display next to the spinner. */ | ||
| message?: string; | ||
| /** The time between each frame of the spinner. |
There was a problem hiding this comment.
It would be helpful to include the unit of time here (milliseconds) — otherwise, the user must review the implementation to understand. For example:
- The time between each frame of the spinner.
+ The time (milliseconds) between each frame of the spinner.
fix(cli/spinner): export type aliases used in public API Ref: #3968
These are just a a few functions similar to
ora's.Whenever I create a CLI app, I find myself not wanting to add a third-party spinner package and adding this code myself. I think this addition to the CLI module is a helpful one for some people. I'm not exactly sure how to add tests for this, since it depends a lot on timing and
stdout, but I'm open to adding tests if someone finds a way.Like #3777, please tell me if this should not be included in the standard lib, and I'll just make a small third party package for it.