-
Notifications
You must be signed in to change notification settings - Fork 934
chore: migrate from ora to nanospinner
#2640
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
Conversation
42f5ff9 to
5c2826f
Compare
5c2826f to
6cc9fcc
Compare
6cc9fcc to
d24109d
Compare
|
There hasn't been any activity on this pull request in the past 3 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. |
|
There hasn't been any activity on this pull request in the past 3 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR migrates the codebase from ora to nanospinner for spinner/loader functionality, aiming to reduce bundle size (nanospinner is 17.5x smaller) and dependencies (1 vs 17). The migration involves updating API calls from succeed()/fail() to success()/error(), changing from direct property assignment (loader.text =) to method calls (loader.update()), and updating type imports across multiple packages.
Key changes:
- Replaced
orawithnanospinnerin package dependencies forcli-tools,cli-doctor, andcli-config-applepackages - Updated loader wrapper in
cli-toolsto use nanospinner's API - Migrated all spinner method calls from ora's API to nanospinner's API across the codebase
Reviewed changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Added nanospinner@^1.0.0 dependency |
| packages/cli/src/commands/init/init.ts | Updated spinner method calls from succeed() to success() |
| packages/cli/src/commands/init/git.ts | Updated spinner method call from succeed() to success() |
| packages/cli-tools/src/loader.ts | Replaced ora with nanospinner, updated NoopLoader implementation |
| packages/cli-tools/package.json | Replaced ora dependency with nanospinner |
| packages/cli-platform-android/src/commands/runAndroid/listAndroidTasks.ts | Updated spinner method calls to use nanospinner API |
| packages/cli-doctor/src/types.ts | Updated Loader type from Ora to Spinner |
| packages/cli-doctor/src/tools/runAutomaticFix.ts | Removed unsupported prefixText option |
| packages/cli-doctor/src/tools/install.ts | Updated fail() to error() |
| packages/cli-doctor/src/tools/healthchecks/*.ts | Updated spinner API calls across all healthcheck modules |
| packages/cli-doctor/src/tools/healthchecks/tests/*.test.ts | Updated test spies to match new API |
| packages/cli-doctor/src/tools/downloadAndUnzip.ts | Changed from property assignment to update() method |
| packages/cli-doctor/src/tools/brewInstall.ts | Updated succeed() to success() |
| packages/cli-doctor/package.json | Replaced ora dependency with nanospinner |
| packages/cli-config-apple/src/tools/runBundleInstall.ts | Updated type import and spinner methods |
| packages/cli-config-apple/src/tools/pods.ts | Updated getLoader calls to use options object, updated spinner methods |
| packages/cli-config-apple/src/tools/installPods.ts | Updated spinner methods and type annotations (has issues) |
| packages/cli-config-apple/package.json | Replaced ora dependency with nanospinner |
| packages/cli-clean/src/clean.ts | Updated spinner method calls |
| docs/init.md | Updated documentation to reference nanospinner instead of ora |
| docs/healthChecks.md | Updated documentation to reference nanospinner API |
Comments suppressed due to low confidence (1)
packages/cli-config-apple/src/tools/installPods.ts:4
- Unused import chalk.
import chalk from 'chalk';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function getLoader(options?: Options): Loader { | ||
| return logger.isVerbose() | ||
| ? new NanospinnerNoop() | ||
| : createSpinner('', options); | ||
| } |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getLoader function incorrectly passes options to createSpinner. According to nanospinner's API, createSpinner takes text as the first parameter and options as the second. However, when getLoader is called with {text: 'some text'}, this implementation passes an empty string as the first parameter and the entire options object (including the text property) as the second parameter. The function should extract the text from options and pass it as the first parameter to createSpinner.
| success() { | ||
| return this; |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The success method signature should accept an optional text parameter to match the nanospinner API, which allows updating the spinner text when calling success(text). The same applies to other methods like error, warn, and info.
| start() { | ||
| return this; |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The start method signature should accept an optional text parameter to match the nanospinner API, which allows starting or restarting the spinner with updated text by calling start(text).
| update() { | ||
| return this; |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update method signature should accept an optional text parameter to match the nanospinner API, which requires passing text when calling update(text) to update the spinner message.
| @@ -1,7 +1,7 @@ | |||
| import fs from 'fs'; | |||
| import execa from 'execa'; | |||
| import type {Ora} from 'ora'; | |||
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import statement still references the old ora package type. This should be updated to import Spinner from nanospinner instead. The type Ora is no longer available after migrating to nanospinner.
| import type {Ora} from 'ora'; | |
| import type {Spinner} from 'nanospinner'; |
| import execa from 'execa'; | ||
| import type {Ora} from 'ora'; | ||
| import pico from 'picocolors'; | ||
| import chalk from 'chalk'; |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import has been changed from pico (picocolors) to chalk, but the code still uses pico throughout the file (lines 30, 32, 86, 97, 98, 130, 131). This will cause runtime errors. Either revert to pico or update all usages to chalk.
|
Welp, with so many changes on main this PR is now completely broken. It was worth a try 😢 |
Summary
This PR migrates the entire repository from
oratonanospinner.nanospinnerhas a 17.5 TIMES smaller footprint [1], [2], and has just one dependency as opposed to 17.Note that
orais not removed from this repo's dependencies, as it remains a dependency ofinquirer. However, it's removed from package dependencies, resulting in faster install and smaller footprint for the end users.Test Plan
To be frank, I relied on TypeScript alone for this change and I don't know this project well enough to test it, especially the Android part.
nanospinneris almost a drop-in replacement fororaso I don't expect major issues though.Checklist
react-nativecheckout (instructions).