feat: add `react-native doctor` #532

lucasbento · 2019-07-15T08:30:42Z

Summary:

This PR fixes #51, it adds a doctor command to verify the softwares/libraries/tools that we depend on and make sure that they match the minimum requirements needed by react-native.

The version for Node.js is not correct as I put it there for testing purposes.

Test Plan:

Uninstall cocoapods or ios-deploy (easiest ones to try);
Run doctor command

TODOS:

patrickkempff · 2019-07-15T10:40:41Z

Wow, this looks awesome!

Inovassist-dev · 2019-07-16T22:15:08Z

Oh my god, I must say, we need that!
Flutter has its own doctor command and it's really good. I wonder how many bugs and bad environment configs could be solved with an on-the-fly "doctor" command!

lucasbento · 2019-07-17T09:01:57Z

@Inovassist-dev: thanks for the feedback on this, that's what we are aiming for!

This week I'm pretty busy, gonna get back to work on this by next week 🙂

rickhanlonii · 2019-08-06T16:34:48Z

Hey @lucasbento, this is amazing! I'd love to help you get this through, let me know what you need!

sahrens · 2019-08-06T16:39:11Z

This would be great, thanks for getting it started!

Thoughts on building this in such a way that we can compose it with other tools and IDEs (e.g. support JSON output instead of logging direct to console). cc @thymikee

cpojer · 2019-08-06T16:47:23Z

Can we call this react-native fix?

Inovassist-dev · 2019-08-06T16:53:54Z

@cpojer I don't think that the name "fix" would be a great idea.
Fluter already has the doctor command and, sincerely, it's a big deal.
As it's not going to necessarily fix React installation, we can call it doctor or repair, just to chase possible errors and, if found, fix them

thymikee · 2019-08-06T16:54:30Z

@sahrens definitely open to that

elicwhite · 2019-08-06T16:57:29Z

I'd love to see this landed and iterated upon. How many features do we need to wait for this to have before landing an MVP?

lucasbento · 2019-08-07T06:20:47Z

Those are the installations that I would like to have in:

Platform agnostic

Current folder is a react-native project (check if react-native is installed)
yarn or npm are in a recent version (need to store the minimum version)
Minimum node version (need to store the minimum version)
Check watchman (need to store the minimum version)

Android

Check if ANDROID_HOME is defined
Android SDK
Android NDK (consider not showing every time as it's not needed for everyone)

iOS

Check Xcode
Check CocoaPods
Check if ios-deploy is installed (need to tell the user somehow that this is only needed with real devices)

Totally discussable though, I would like to have this on master as soon as possible so it's easier for people to add more stuff.

@rickhanlonii @sahrens: a review on the source would be very much appreciated, I'm trying to build something as modular as possible, the JSON output was already asked on #51 and I'm definitely on board with that.

The first thing that I need help with is a little review and adding more installation checks, if anyone of you want to help with that then let's do it!

tabrindle · 2019-08-07T09:51:42Z

I realize that you are looking to add more functionality in the future, but right now this is exactly the feature set of Solidarity https://github.com/infinitered/solidarity Might we consider using this and combining efforts on the automatic fix part on the wishlist?

rickhanlonii · 2019-08-07T10:05:31Z

@tabrindle Aolidariy is great! Will support things like checking for IDE installations, android SDKs, xcode versions, vscode/intellij/android studio plugins?

tabrindle · 2019-08-07T10:48:14Z

Solidarity can do anything envinfo does, as it uses it under the hood.

rickhanlonii

Overall looks great, I really like the way the health checks are handled so that it's easy to add a new check. I think we can go with this and iterate 👍

rickhanlonii · 2019-08-07T10:36:40Z

packages/cli/src/commands/doctor/checkInstallation.js

+};
+
+const doesSoftwareNeedToBeFixed = ({version, versionRange}) =>
+  version === 'Not Found' || !semver.satisfies(version, versionRange);


Note that semver can throw here if for some reason one of the versions are invalid. e.g. for me, I'm getting the string "Using globally installed version of Yarn"

We'll at least need to try/catch here, but in the catch we may want to log a quality error and consider crashing because if we don't get a semver by this point that the individual issue needs to diagnose why they don't have a semver

How did you install yarn? I want to reproduce that locally.

Probably npm install -g yarn

packages/cli/src/commands/doctor/doctor.js

rickhanlonii · 2019-08-07T10:40:47Z

packages/cli/src/commands/doctor/doctor.js

+
+export default (async function runDoctor() {
+  const Loader = getLoader();
+  const loader = new Loader();


Nit: naming this a "loader" when it's not really showing a loading message and is mostly only a logger is hard to read (I recognize that this is called a loader elsewhere so we should either change all usages or stay consistent)

packages/cli/src/commands/doctor/doctor.js

rickhanlonii · 2019-08-07T10:48:01Z

packages/cli/src/commands/doctor/runAutomaticFix.js

+      try {
+        await issueToFix.runAutomaticFix({loader: issueSpinner});
+      } catch (error) {
+        // TODO: log the error in a meaningful way


Would console.error be better than nothing for now?

Didn't pay much attention to this part yet, it's hard to log an error right now as the loader is on top of everything, so first it needs to stop the loader, log an error and then continue to the next automatic fix.

packages/cli/src/commands/doctor/doctor.js

Inovassist-dev · 2019-08-07T12:15:17Z

@tabrindle, I think it would be nice to have something like Solidarity already integrated with React Native, dont you think?

lucasbento · 2019-08-08T07:37:11Z

@rickhanlonii: thank you so much for the review! gonna check it out.

I realize that you are looking to add more functionality in the future, but right now this is exactly the feature set of Solidarity infinitered/solidarity Might we consider using this and combining efforts on the automatic fix part on the wishlist?

@tabrindle: I checked solidarity before starting this and I struggled to understand how it would be used on the core of the CLI, perhaps we should discuss its possibilities somewhere else?

rickhanlonii · 2019-08-28T10:44:53Z

@lucasbento do you want to get this in an start iterating?

thymikee · 2019-09-03T09:17:56Z

I like the direction where it's going. Let's make sure to mark it as experimental (e.g. in the description and the docs, or add docs later) and it doesn't break stable commands. And ship it

packages/cli/src/commands/doctor/versionRanges.js

tido64 · 2019-09-03T12:17:49Z

packages/cli/src/commands/doctor/healthchecks/cocoaPods.js

+  runAutomaticFix: async ({loader}) => {
+    try {
+      // First attempt to install `cocoapods`
+      await execa('gem', ['install', 'cocoapods']);


Should we rely on Homebrew/MacPorts to avoid having to invoke sudo? Or at least use them if they are installed.

we have a helper function for that, let's use it :) https://github.com/react-native-community/cli/blob/master/packages/cli/src/tools/installPods.js

tido64 · 2019-09-03T12:30:06Z

packages/cli/src/commands/doctor/doctor.js

+
+        process.exit(0);
+      } catch (err) {
+        // TODO: log error


Is this going to be done now or in a separate PR?

tido64 · 2019-09-03T12:37:08Z

packages/cli/src/commands/doctor/healthchecks/packageManagers.js

+  // or if we can't identify that the user uses yarn or npm
+  visible:
+    packageManager === PACKAGE_MANAGERS.YARN || packageManager === undefined,
+  runAutomaticFix: () => console.log('should fix node'),


Is this intentional? Isn't Yarn installed separately?

tido64 · 2019-09-03T12:39:04Z

packages/cli/src/commands/doctor/healthchecks/watchman.js

+      versionRange: versionRanges.WATCHMAN,
+    }),
+  }),
+  runAutomaticFix: () => console.log('should fix watchman'),


Is this placeholder text?

sweggersen · 2019-09-03T12:26:44Z

packages/cli/src/commands/doctor/healthchecks/androidHomeEnvVariable.js

+
+const label = 'ANDROID_HOME';
+
+const iosDeploy = {


sweggersen · 2019-09-03T12:26:53Z

packages/cli/src/commands/doctor/healthchecks/androidNDK.js

+import versionRanges from '../versionRanges';
+import {doesSoftwareNeedToBeFixed} from '../checkInstallation';
+
+const iosDeploy = {


sweggersen · 2019-09-03T12:26:58Z

packages/cli/src/commands/doctor/healthchecks/androidSDK.js

+import versionRanges from '../versionRanges';
+import {doesSoftwareNeedToBeFixed} from '../checkInstallation';
+
+const iosDeploy = {


sweggersen · 2019-09-03T12:31:24Z

packages/cli/src/commands/doctor/healthchecks/androidHomeEnvVariable.js

+
+// List of answers on how to set `ANDROID_HOME` for each platform
+const URLS = {
+  darwin: 'https://stackoverflow.com/a/28296325/4252781',


Maybe it would be smart to inline these answers

thymikee

Let's fix the remaining stuff in followup PRs 👍

lucasbento mentioned this pull request Jul 15, 2019

Health check command #51

Closed

rickhanlonii approved these changes Aug 7, 2019

View reviewed changes

thymikee marked this pull request as ready for review September 3, 2019 09:15

thymikee requested review from Esemesek, grabbou and thymikee as code owners September 3, 2019 09:15

tido64 reviewed Sep 3, 2019

View reviewed changes

packages/cli/src/commands/doctor/versionRanges.js Outdated Show resolved Hide resolved

tido64 reviewed Sep 3, 2019

View reviewed changes

sweggersen reviewed Sep 3, 2019

View reviewed changes

Lucas Bento added 4 commits September 3, 2019 15:27

Update semver version

7448960

Add doctor command

062a0ee

Add basic version ranges

74f8181

Add functions to check software installations

03c099e

Lucas Bento and others added 25 commits September 3, 2019 15:27

Add automatic fix for CocoaPods

762ab51

Log an URL to install ios-deploy if the automatic fix fails

248065a

Change manual installation messages

70810e1

Add manual installation message to Xcode

9df0e24

Remove key listening after the user chooses an option

1ecc788

Decrease delay on Node.js installation and put correct loader call

76e0069

Add small comment on automatic fix run

1899e1f

Improve the handling of cocoapods installation with sudo

63de535

Use healthcheck instead of issue

04d29fc

Move files to inside of healthchecks

ed1c796

Rename issue to healthcheck

579f1a0

Move overall stats to its own function

7d883d8

Only show categories that have needsToBeFixed flag

7f7307f

Only show iOS category on mac

a1dabf3

Add ANDROID_HOME check

63b1b26

Pass in environmentInfo to runAutomaticFix function

5324ab8

Rename ANDROID_HOME env variable healthcheck

775903f

Add checks for Android SDK and NDK

4a85aa1

Run Android NDK healthcheck only if --contributor argument is passed

342c1e6

Fix issues based on what has been chosen

7750aec

Add manual step to Node.js healthcheck

22a7428

Add --fix option

5213840

Fix wrong version of Android SDK

07a7b7c

add EXPERIMENTAL notice

c6d55d3

add missing methods to OraMock

4f3f016

thymikee force-pushed the feat/doctor branch from 5770c61 to 4f3f016 Compare September 3, 2019 13:30

thymikee added 2 commits September 3, 2019 15:32

fix command-exists missing

d940b3d

fix node range

6424490

thymikee approved these changes Sep 3, 2019

View reviewed changes

thymikee merged commit 9139af0 into react-native-community:master Sep 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `react-native doctor` #532

feat: add `react-native doctor` #532

lucasbento commented Jul 15, 2019 •

edited

Loading

patrickkempff commented Jul 15, 2019

Inovassist-dev commented Jul 16, 2019

lucasbento commented Jul 17, 2019

rickhanlonii commented Aug 6, 2019

sahrens commented Aug 6, 2019

cpojer commented Aug 6, 2019

Inovassist-dev commented Aug 6, 2019 •

edited

Loading

thymikee commented Aug 6, 2019

elicwhite commented Aug 6, 2019

lucasbento commented Aug 7, 2019

tabrindle commented Aug 7, 2019

rickhanlonii commented Aug 7, 2019

tabrindle commented Aug 7, 2019

rickhanlonii left a comment

rickhanlonii Aug 7, 2019

lucasbento Aug 14, 2019

rickhanlonii Aug 28, 2019

rickhanlonii Aug 7, 2019

rickhanlonii Aug 7, 2019

lucasbento Aug 14, 2019

Inovassist-dev commented Aug 7, 2019

lucasbento commented Aug 8, 2019

rickhanlonii commented Aug 28, 2019

thymikee commented Sep 3, 2019

tido64 Sep 3, 2019

thymikee Sep 3, 2019

tido64 Sep 3, 2019

tido64 Sep 3, 2019

tido64 Sep 3, 2019

sweggersen Sep 3, 2019

sweggersen Sep 3, 2019

sweggersen Sep 3, 2019

sweggersen Sep 3, 2019

thymikee left a comment

feat: add react-native doctor #532

feat: add react-native doctor #532

Conversation

lucasbento commented Jul 15, 2019 • edited Loading

Summary:

Test Plan:

TODOS:

patrickkempff commented Jul 15, 2019

Inovassist-dev commented Jul 16, 2019

lucasbento commented Jul 17, 2019

rickhanlonii commented Aug 6, 2019

sahrens commented Aug 6, 2019

cpojer commented Aug 6, 2019

Inovassist-dev commented Aug 6, 2019 • edited Loading

thymikee commented Aug 6, 2019

elicwhite commented Aug 6, 2019

lucasbento commented Aug 7, 2019

Platform agnostic

Android

iOS

tabrindle commented Aug 7, 2019

rickhanlonii commented Aug 7, 2019

tabrindle commented Aug 7, 2019

rickhanlonii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Inovassist-dev commented Aug 7, 2019

lucasbento commented Aug 8, 2019

rickhanlonii commented Aug 28, 2019

thymikee commented Sep 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thymikee left a comment

Choose a reason for hiding this comment

feat: add `react-native doctor` #532

feat: add `react-native doctor` #532

lucasbento commented Jul 15, 2019 •

edited

Loading

Inovassist-dev commented Aug 6, 2019 •

edited

Loading