fix(deps): replace ora and log-symbols with tiny dependency picospinner
#7026
Conversation
| @@ -15,8 +15,7 @@ const preserveScripts = new Set([ | |||
| 'prepublishOnly', | |||
| ]) | |||
|
|
|||
| let spinner = ora({ | |||
| spinner: 'star', | |||
There was a problem hiding this comment.
this one is a CI script so I didn't bother preserving the exact styling
| const DOTS_SPINNER = { | ||
| interval: 80, | ||
| frames: ['⠋', '⠙', '⠹', '⠸', '⠼', '⠴', '⠦', '⠧', '⠇', '⠏'], | ||
| } |
There was a problem hiding this comment.
This was the default in ora, which we were implicitly using here.
| runCommand(settings.command, { env: settings.env, spinner, cwd }) | ||
| if (settings.command) { | ||
| runCommand(settings.command, { env: settings.env, spinner, cwd }) | ||
| } |
There was a problem hiding this comment.
If you scroll up you'll see we were already doing this in one branch but not the other. Adding some type safety uncovered this.
| // @ts-expect-error TS(7006) FIXME: Parameter 'writeStream' implicitly has an 'any' ty... Remove this comment to see the full error message | ||
| const pipeDataWithSpinner = (writeStream, chunk) => { | ||
| if (spinner && spinner.isSpinning) { | ||
| const pipeDataWithSpinner = (writeStream: NodeJS.WriteStream, chunk: any) => { |
There was a problem hiding this comment.
To be clear, chunk is literally typed as any in the NodeJS types. This isn't a lazy type 😄.
| if (spinner?.isSpinning) { | ||
| spinner.clear() | ||
| spinner.isSilent = true | ||
| } | ||
| writeStream.write(chunk, () => { | ||
| if (spinner && spinner.isSpinning) { | ||
| spinner.isSilent = false | ||
| spinner.render() | ||
| if (spinner?.isSpinning) { | ||
| spinner.start() | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Feel free to compare the docs for ora and picospinner and double-check me here, but I think this should reproduce the behaviour. Of course I'll poke at it manually before considering merging.
Noooooooo |
|
@serhalp i would actually recommend using nanospinner or picospinner (if you need a little more control of rendering) particularly nanospinner has been adopted widely elsewhere so we can benefit from npm de-duplication by using it. its also generally faster/smaller (though these are all spinners so they're about as big/fast as each other...) and it uses picocolors, which again has been widely adopted as the colours library of choice, so will de-dupe well |
serhalp
force-pushed
the
fix/replace-ora
branch
from
f87c45e to
e932659
Compare
This should reduce our package size. I also added some missing types along the way.
serhalp
force-pushed
the
fix/replace-ora
branch
from
e932659 to
82143f3
Compare
| // @ts-expect-error TS(7023) FIXME: 'listSites' implicitly has return type 'any' becau... Remove this comment to see the full error message | ||
| export const listSites = async ({ api, options }): SiteInfo[] => { | ||
| export const listSites = async ({ api, options }): Promise<SiteInfo[]> => { |
There was a problem hiding this comment.
I have the rest of this fixed in another draft PR. I just needed this to unblock errors elsewhere.
serhalp
changed the title
ora and log-symbols with tiny dependency yocto-spinnerora and log-symbols with tiny dependency picospinner
| if (!options.json) { | ||
| // @ts-expect-error TS(2345) FIXME: Argument of type '{ spinner: Ora | undefined; }' i... Remove this comment to see the full error message | ||
| if (spinner) { |
There was a problem hiding this comment.
These are equivalent, but this is less leaky and makes TS happy. We only define a spinner (upstream) when --json isn't true.
| @@ -45,6 +45,11 @@ const languages = [ | |||
| { name: 'Rust', value: 'rust' }, | |||
| ] | |||
|
|
|||
| const MOON_SPINNER = { | |||
Summary
This should reduce our package size a bit, which is huge. See https://github.com/es-tooling/module-replacements/blob/main/docs/modules/ora.md.
I also added some missing types along the way.