Skip to content
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

feat (doctor): add packager healthcheck to report if packager is running correctly #1941

Merged

Conversation

tarunrajput
Copy link
Contributor

@tarunrajput tarunrajput commented May 24, 2023

Summary:

Part of #1936
Added packager healthcheck in cli-doctor to report if packager is running correctly

Test Plan:

  1. Build cli codebase using : node ./scripts/build.js && yarn build:debugger
  2. Link the cli-doctor using : yarn link
  3. In the example React Native app(Awesome Project), run yarn link "@react-native-community/cli-doctor"
  4. Run the CLI Doctor using : npx @react-native-community/cli doctor

Failure:

⠏ Running diagnostics...
Common
 ✓ Node.js - Required to execute JavaScript code
 ✓ yarn - Required to install NPM dependencies
 ● Metro - Metro Bundler is not running
 ✓ Watchman - Used for watching changes in the filesystem when in development mode

Android
 ✖ Adb - No devices and/or emulators connected. Please create emulator with Android Studio or connect Android device.
 ✓ JDK - Required to compile Java code
 ✓ Android Studio - Required for building and installing your app on Android
 ✖ Android SDK - Required for building and installing your app on Android
   - Versions found: N/A
   - Version supported: 33.0.0
 ✖ ANDROID_HOME - Environment variable that points to your Android SDK installation

iOS
 ✓ Xcode - Required for building and installing your app on iOS
 ✓ Ruby
 ✓ CocoaPods - Required for installing iOS dependencies
 ● ios-deploy - Required for installing your app on a physical device with the CLI
 ✖ .xcode.env - File to customize Xcode environment

Errors:   4
Warnings: 2

Success

⠋ Running diagnostics...
Common
 ✓ Node.js - Required to execute JavaScript code
 ✓ yarn - Required to install NPM dependencies
 ✓ Metro - Required for bundling the JavaScript code
 ✓ Watchman - Used for watching changes in the filesystem when in development mode

Android
 ✖ Adb - No devices and/or emulators connected. Please create emulator with Android Studio or connect Android device.
 ✓ JDK - Required to compile Java code
 ✓ Android Studio - Required for building and installing your app on Android
 ✖ Android SDK - Required for building and installing your app on Android
   - Versions found: N/A
   - Version supported: 33.0.0
 ✖ ANDROID_HOME - Environment variable that points to your Android SDK installation

iOS
 ✓ Xcode - Required for building and installing your app on iOS
 ✓ Ruby
 ✓ CocoaPods - Required for installing iOS dependencies
 ● ios-deploy - Required for installing your app on a physical device with the CLI
 ✖ .xcode.env - File to customize Xcode environment

Errors:   4
Warnings: 1

Usage
 › Press f to try to fix issues.
 › Press e to try to fix errors.
 › Press w to try to fix warnings.
 › Press Enter to exit.

@adamTrz
Copy link
Collaborator

adamTrz commented May 24, 2023

Thanks for the PR @tarunrajput
One small concern from my side - not 100% sure that it should be treated as an error... Users can run doctor when packager is not running, just to make some diagnostics.
Maybe warning would be enough in this case? 🤔
Wdyt?

@robhogan
Copy link
Collaborator

Driveby wording nit: "Metro" is the name of the bundler, not "Metro Packager".

"Packager" (and the packager-status API) is a legacy of Metro's predecessor from before 2017.

@tarunrajput tarunrajput force-pushed the feat/add-packager-healthcheck branch from a8e077a to 7b53258 Compare May 24, 2023 15:28
@tarunrajput
Copy link
Contributor Author

@adamTrz, I agree that treating it as a warning instead of an error would be more appropriate.
@robhogan, Thanks for pointing that out. I have updated the PR to reflect the correct name.

Copy link
Contributor

@arushikesarwani94 arushikesarwani94 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @tarunrajput.
Changes look good and sufficient warning for the packager to me.

Copy link
Collaborator

@szymonrybczak szymonrybczak left a comment

Choose a reason for hiding this comment

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

Thank you for contribution! 🚀

@adamTrz adamTrz merged commit a2e90fd into react-native-community:main May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants