Migrate the rest of doctor command to TypeScript #722

thib92 · 2019-09-14T14:24:32Z

Summary:

This is the last PR to move the doctor command to TypeScript. This one was quite a long one because I needed to unify the type definitions of all the functions manipulating the type checks, and it can be a little bit confusing (at least to me).

So I hope with this, all the functions and their roles in the healthchecks will be easier to read.

Feel free to comment if anything feels off.

Migrate the rest of doctor command to TypeScript
Unify type definitions for the command
Remove Flow typings

Test Plan:

CI is green ✅
Everything makes sense

Unify type definitions for the command Remove Flow typings

packages/cli/src/commands/doctor/doctor.ts

thib92 · 2019-09-14T14:27:18Z

packages/cli/src/commands/doctor/doctor.ts

-  );
+  const healthchecksPerCategory = await iterateOverCategories(Object.values(
+    getHealthchecks(options),
+  ).filter(category => category !== undefined) as HealthcheckCategory[]);


Same here, the as part is necessary, but could use type predicates.

thib92 · 2019-09-14T14:27:45Z

packages/cli/src/commands/doctor/doctor.ts

-  const onKeyPress = async key => {
-    // $FlowFixMe
+  const onKeyPress = async (key: string) => {
+    // @ts-ignore
    process.stdin.setRawMode(false);


Does stdin.setRawMode really exist? It's not in the typings.

It exists in TTY, which is our primary use case, so yep: https://nodejs.org/api/tty.html#tty_readstream_setrawmode_mode
Please check if typeof process.stdin.setRawMode === "function" before accessing it. We should add it to every call of this method at some point.

My bad, it's optional. Thanks!

thib92 · 2019-09-14T14:28:23Z

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

  healthchecks,
  automaticFixLevel,
  stats,
-  loader,


I saw here that this function doesn't use its loader argument, but uses ora directly from the import. Is this intended?

if loader is unused, please remove it from the type def as well

Remove useless arguments

thymikee · 2019-09-15T17:51:52Z

packages/cli-types/src/index.ts

@@ -13,14 +13,16 @@ import {

 export type InquirerPrompt = any;

+export type CommandFunction = (


We could make it a parametrized type and pass the param to args, what do you think?

thymikee · 2019-09-15T17:53:15Z

packages/cli/src/commands/doctor/doctor.ts

-  ctx: ConfigT,
-  options: FlagsT,
-) {
+export default (async (_, __, options: FlagsT) => {


Use something like CommandFunction<FlagsT> maybe?

packages/cli/src/commands/doctor/doctor.ts

thymikee · 2019-09-15T19:39:28Z

packages/cli/src/commands/doctor/doctor.ts

-  const onKeyPress = async key => {
-    // $FlowFixMe
+  const onKeyPress = async (key: string) => {
+    // @ts-ignore
    process.stdin.setRawMode(false);


It exists in TTY, which is our primary use case, so yep: https://nodejs.org/api/tty.html#tty_readstream_setrawmode_mode
Please check if typeof process.stdin.setRawMode === "function" before accessing it. We should add it to every call of this method at some point.

thymikee · 2019-09-15T19:43:31Z

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

  // Remove the fix options from screen
-  // $FlowFixMe
+  // @ts-ignore


instead of ignore, please add if (process.stdout.isTTY) {} check, or make sure moveCursor and clearScreenDown exist

thymikee · 2019-09-15T19:44:51Z

packages/cli/src/commands/doctor/types.ts

+};
+
+export type Healthchecks = {
+  common: HealthcheckCategory;


Please rename to HealthChecks and HealthCheckCategory to stay consistent consistency

Co-Authored-By: Michał Pierzchała <thymikee@gmail.com>

thib92 · 2019-09-16T09:14:31Z

All done @thymikee, thanks for the review!

thymikee · 2019-09-16T09:25:32Z

cc @lucasbento

lucasbento · 2019-09-16T09:32:48Z

packages/cli/src/commands/doctor/doctor.ts

 import chalk from 'chalk';
+// @ts-ignore


Noobie question: shouldn't we type these instead of just adding it to ignore?

Yep we should. I might give it a try, but there is a pending WIP PR on the repo. Would probably be worth being put on DefinitelyTyped

lucasbento · 2019-09-16T09:35:10Z

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

-  ERRORS: 'ERRORS',
-  WARNINGS: 'WARNINGS',
-};
+export enum AUTOMATIC_FIX_LEVELS {


lucasbento

Looks awesome, thank you for this @thib92!

lucasbento · 2019-09-16T09:36:43Z

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

-  process.stdout.clearScreenDown();
+  if (process.stdout.isTTY) {
+    // @ts-ignore
+    process.stdout.moveCursor(0, -6);


Those should be part of readline as they are apparently not typed under process.stdout: DefinitelyTyped/DefinitelyTyped#31505.

I can change those later as there's no harm on ignoring this.

lucasbento · 2019-09-16T09:40:32Z

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


-export {AUTOMATIC_FIX_LEVELS};
-export default async ({
+interface RunAutomaticFixArgs {


@thib92: small question, when do you use type and interface?

There's a small difference between the two, but to be fair I usually use type because you can union them, type functions, etc. I'm not sure what the advantages of interface is though. I saw you used interface before so I used them where I could

Cool, thanks for replying!

thib92 · 2019-09-16T10:00:45Z

Thanks for the merge. @thymikee @lucasbento if you need help on other stuff, please send me a message! I'd love to contribute more on the project! I'll be looking at issues that I could take 😄

thymikee · 2019-09-16T10:01:59Z

@thib92 thank you! ❤️

lucasbento · 2019-09-16T10:10:12Z

@thib92: thanks for the contributions!

We have a good list of issues that we need to work on at #694 in order to get doctor out of experimental, if you are feeling adventurous you can take one and open a PR 🙂

thib92 · 2019-09-16T10:14:03Z

@lucasbento thanks a lot, will take a look!

Migrate the rest of doctor command to TypeScript

7b650f3

Unify type definitions for the command Remove Flow typings

thib92 requested review from Esemesek, grabbou and thymikee as code owners September 14, 2019 14:24

thib92 commented Sep 14, 2019

View reviewed changes

packages/cli/src/commands/doctor/doctor.ts Show resolved Hide resolved

Remove useless falsy condition

89b0bf7

thib92 commented Sep 14, 2019

View reviewed changes

thib92 added 3 commits September 14, 2019 16:29

Change any type to number

08bde8d

Use CommandFunction type

9e06778

Remove useless arguments

Export enum syntax

d66b1a0

thymikee requested changes Sep 15, 2019

View reviewed changes

thib92 and others added 2 commits September 16, 2019 11:10

Update packages/cli/src/commands/doctor/doctor.ts

aac74ca

Co-Authored-By: Michał Pierzchała <thymikee@gmail.com>

PR comments @thymikee

f0848fc

thymikee approved these changes Sep 16, 2019

View reviewed changes

lucasbento reviewed Sep 16, 2019

View reviewed changes

lucasbento approved these changes Sep 16, 2019

View reviewed changes

thymikee merged commit 85198d2 into react-native-community:master Sep 16, 2019

lucasbento reviewed Sep 16, 2019

View reviewed changes

thib92 deleted the typescript-command-doctor branch September 16, 2019 09:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate the rest of doctor command to TypeScript #722

Migrate the rest of doctor command to TypeScript #722

thib92 commented Sep 14, 2019

thib92 Sep 14, 2019

thib92 Sep 14, 2019

thymikee Sep 15, 2019

thib92 Sep 16, 2019

thib92 Sep 14, 2019

thymikee Sep 16, 2019

thymikee Sep 15, 2019

thib92 Sep 16, 2019

thymikee Sep 15, 2019

thymikee Sep 15, 2019

thymikee Sep 15, 2019

thymikee Sep 15, 2019

thib92 commented Sep 16, 2019

thymikee commented Sep 16, 2019

lucasbento Sep 16, 2019

thib92 Sep 16, 2019

lucasbento Sep 16, 2019

lucasbento left a comment

lucasbento Sep 16, 2019

lucasbento Sep 16, 2019

thib92 Sep 16, 2019

lucasbento Sep 16, 2019

thib92 commented Sep 16, 2019

thymikee commented Sep 16, 2019

lucasbento commented Sep 16, 2019

thib92 commented Sep 16, 2019

		@@ -13,14 +13,16 @@ import {

		export type InquirerPrompt = any;

		export type CommandFunction = (

Migrate the rest of doctor command to TypeScript #722

Migrate the rest of doctor command to TypeScript #722

Conversation

thib92 commented Sep 14, 2019

Summary:

Test Plan:

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thib92 commented Sep 16, 2019

thymikee commented Sep 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasbento 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

thib92 commented Sep 16, 2019

thymikee commented Sep 16, 2019

lucasbento commented Sep 16, 2019

thib92 commented Sep 16, 2019