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

fix(doctor): pass config to health checks #1850

Merged

Conversation

tido64
Copy link
Contributor

@tido64 tido64 commented Feb 28, 2023

Summary:

Resolves #1588

Test Plan:

The doctor command should work as before:

node /path/to/react-native-cli/packages/cli/build/bin.js doctor

@tido64
Copy link
Contributor Author

tido64 commented Feb 28, 2023

@adamTrz: Let me know what you think of this. I know you made an attempt in #1669, but I think this is simpler. Admittedly, I have no idea why some commands are considered detached, so this may not work as I thought.

Edit: I'll update the tests if this approach looks fine to you.

@adamTrz
Copy link
Collaborator

adamTrz commented Mar 3, 2023

Hey @tido64 thanks for PR, code looks good and simplifies things a bit :)

As for detached mode - actually I don't think it works even at main right now... At least not for me 🤷

When I try to run doctor command not from react-native project I got an error: We couldn't find a package.json in your project. It's being thrown here when looking for project root at xcodeEnv healthcheck:

export default {
label: '.xcode.env',
description: 'File to customize Xcode environment',
getDiagnostics: async () => {
const projectRoot = findProjectRoot();

We could catch the error in similar way like androidSDK health check does (this would have also be done for runAutomaticFix bit):

let projectRoot = '';
    try {
      projectRoot = findProjectRoot();
    } catch (error) {
      logger.log(); // for extra space
      logger.warn(
        "We couldn't find a package.json in this directory, .xcode.env check may fail. Doctor works best in a React Native project root.",
      );
    }

(note that const allPathsHasXcodeEnvFile = findPodfilePaths(projectRoot lookup that's being performed afterwards can still succeed if there's a project in subfolder that does have .xcode.env file present)

Here's doctor output from folder containing some random react-native projects (after proposed fix):

❯ ls -alt                                    
total 16
drwxr-xr-x  33 adamtrzcinski  staff  1056 Mar  3 13:18 cli
drwxr-xr-x  25 adamtrzcinski  staff   800 Mar  3 13:16 Foo
drwxr-xr-x  10 adamtrzcinski  staff   320 Mar  3 11:14 .
drwxr-xr-x  30 adamtrzcinski  staff   960 Feb 24 12:12 cli-clean
drwxr-xr-x  31 adamtrzcinski  staff   992 Feb 17 16:41 MyAppFromTemplate
drwxr-xr-x  11 adamtrzcinski  staff   352 Feb  1 19:05 ExpoStickerSmash
drwxr-xr-x@ 14 adamtrzcinski  staff   448 Feb  1 13:22 ..
drwxr-xr-x  24 adamtrzcinski  staff   768 Jan 20 15:52 BarUnlinked
drwxr-xr-x   4 adamtrzcinski  staff   128 Jan 19 20:10 TurboModulesGuide
-rw-r--r--@  1 adamtrzcinski  staff  6148 Oct 28 13:10 .DS_Store
Documents/git/OSS 
❯ node ./cli/packages/cli/build/bin.js doctor
⠋ Running diagnostics...
warn We couldn't find a package.json in this directory. Android SDK checks may fail. Doctor works best in a React Native project root.

warn We couldn't find a package.json in this directory, .xcode.env check may fail. Doctor works best in a React Native project root.
Common
 ✓ Node.js - Required to execute JavaScript code
 ✓ yarn - Required to install NPM dependencies
 ✓ npm - Required to install NPM dependencies
 ● Watchman - Used for watching changes in the filesystem when in development mode

Android
 ✓ 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: 29.0.0, 29.0.1, 29.0.2, 29.0.3, 30.0.0, 30.0.1, 30.0.2, 30.0.3, 31.0.0, 32.0.0, 32.1.0, 33.0.2
   - Version supported: Not Found
 ✓ ANDROID_HOME - Environment variable that points to your Android SDK installation

iOS
 ✓ Xcode - Required for building and installing your app on iOS
 ✓ Ruby - Required for installing iOS dependencies
 ✓ 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:   2
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.
 

But that's a bit of an off-topic 😉

When I apply your changes tho doctor does nothing in detached mode - no logs no errors, nothing...
It looks like it fails silently somewhere here:

try {
const config = loadConfig();

Warnings are visible when we add --verbose option:
image

Please let me know if what I wrote makes any sense, not super familiar with this detached mode so maybe it should work differently. 🙂

Comment on lines -38 to -41
.reduce(
(previousValue: boolean, currentValue: boolean) =>
previousValue && currentValue,
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why this wasn't discovered earlier but it is missing the initial value.

@tido64
Copy link
Contributor Author

tido64 commented Mar 3, 2023

Hey @tido64 thanks for PR, code looks good and simplifies things a bit :)

Thanks for taking a look. Since you agree with this approach, I cleaned up the rest of the issues. Would be great if you could take another look.

@adamTrz
Copy link
Collaborator

adamTrz commented Mar 3, 2023

Hey @tido64 thanks for PR, code looks good and simplifies things a bit :)

Thanks for taking a look. Since you agree with this approach, I cleaned up the rest of the issues. Would be great if you could take another look.

Splendid! Detached mode works find outside of RN project 🎉

@adamTrz adamTrz merged commit b7954c9 into react-native-community:main Mar 3, 2023
@tido64 tido64 deleted the tido/pass-config-to-health-checks branch March 4, 2023 15:13
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.

doctor: refactor the fixers so we can forward options and avoid duplicating efforts
2 participants