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

Migrate the rest of doctor command to TypeScript #722

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions packages/cli-types/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,16 @@ import {

export type InquirerPrompt = any;

export type CommandFunction = (
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call!

argv: Array<string>,
ctx: Config,
args: Object,
) => Promise<void> | void;

export interface Command {
name: string;
description?: string;
func: (
argv: Array<string>,
ctx: Config,
args: Object,
) => Promise<void> | void;
func: CommandFunction;
options?: Array<{
name: string;
description?: string;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
// @flow
import chalk from 'chalk';
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

import envinfo from 'envinfo';
import {logger} from '@react-native-community/cli-tools';
// $FlowFixMe - converted to TS
import {getHealthchecks, HEALTHCHECK_TYPES} from './healthchecks';
// $FlowFixMe - converted to TS
import {getLoader} from '../../tools/loader';
// $FlowFixMe - converted to TS
import printFixOptions, {KEYS} from './printFixOptions';
import runAutomaticFix, {AUTOMATIC_FIX_LEVELS} from './runAutomaticFix';
import type {ConfigT} from 'types';
import type {HealthCheckInterface} from './types';

const printCategory = ({label, key}) => {
import {CommandFunction} from '@react-native-community/cli-types';
import {
HealthcheckCategory,
thib92 marked this conversation as resolved.
Show resolved Hide resolved
HealthCheckCategoryResult,
HealthCheckResult,
} from './types';

const printCategory = ({label, key}: {label: string; key: number}) => {
if (key > 0) {
logger.log();
}
Expand All @@ -25,12 +26,7 @@ const printIssue = ({
needsToBeFixed,
isRequired,
description,
}: {
label: string,
needsToBeFixed: boolean,
isRequired: boolean,
description: string,
}) => {
}: HealthCheckResult) => {
const symbol = needsToBeFixed
? isRequired
? chalk.red('✖')
Expand All @@ -40,21 +36,23 @@ const printIssue = ({
logger.log(` ${symbol} ${label}${needsToBeFixed ? ': ' + description : ''}`);
};

const printOverallStats = ({errors, warnings}) => {
const printOverallStats = ({
errors,
warnings,
}: {
errors: number;
warnings: number;
}) => {
logger.log(`\n${chalk.bold('Errors:')} ${errors}`);
logger.log(`${chalk.bold('Warnings:')} ${warnings}`);
};

type FlagsT = {
fix: boolean | void,
contributor: boolean | void,
fix: boolean | void;
contributor: boolean | void;
};

export default (async function runDoctor(
argv: Array<string>,
ctx: ConfigT,
options: FlagsT,
) {
export default (async (_, __, options: FlagsT) => {
Copy link
Member

Choose a reason for hiding this comment

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

Use something like CommandFunction<FlagsT> maybe?

const Loader = getLoader();
const loader = new Loader();

Expand All @@ -76,10 +74,7 @@ export default (async function runDoctor(
const iterateOverHealthChecks = async ({
label,
healthchecks,
}: {
label: string,
healthchecks: Array<HealthCheckInterface>,
}) => ({
}: HealthcheckCategory): Promise<HealthCheckCategoryResult> => ({
label,
healthchecks: (await Promise.all(
healthchecks.map(async healthcheck => {
Expand Down Expand Up @@ -108,23 +103,22 @@ export default (async function runDoctor(
: undefined,
};
}),
)).filter(Boolean),
)).filter(healthcheck => healthcheck !== undefined) as HealthCheckResult[],
thymikee marked this conversation as resolved.
Show resolved Hide resolved
});

// Remove all the categories that don't have any healthcheck with `needsToBeFixed`
// so they don't show when the user taps to fix encountered issues
const removeFixedCategories = categories =>
const removeFixedCategories = (categories: HealthCheckCategoryResult[]) =>
categories.filter(category =>
category.healthchecks.some(healthcheck => healthcheck.needsToBeFixed),
);

const iterateOverCategories = categories =>
// $FlowFixMe - bad Object.values typings
const iterateOverCategories = (categories: HealthcheckCategory[]) =>
Promise.all(categories.map(iterateOverHealthChecks));

const healthchecksPerCategory = await iterateOverCategories(
Object.values(getHealthchecks(options)),
);
const healthchecksPerCategory = await iterateOverCategories(Object.values(
getHealthchecks(options),
).filter(category => category !== undefined) as HealthcheckCategory[]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


loader.stop();

Expand All @@ -136,15 +130,17 @@ export default (async function runDoctor(
healthchecksPerCategory.forEach((issueCategory, key) => {
printCategory({...issueCategory, key});

issueCategory.healthchecks.map(healthcheck => {
issueCategory.healthchecks.forEach(healthcheck => {
printIssue(healthcheck);

if (healthcheck.type === HEALTHCHECK_TYPES.WARNING) {
return stats.warnings++;
stats.warnings++;
return;
}

if (healthcheck.type === HEALTHCHECK_TYPES.ERROR) {
return stats.errors++;
stats.errors++;
return;
}
});
});
Expand All @@ -161,13 +157,14 @@ export default (async function runDoctor(
});
}

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, it's optional. Thanks!

process.stdin.removeAllListeners('data');

if (key === KEYS.EXIT || key === '\u0003') {
return process.exit(0);
process.exit(0);
return;
}

if (
Expand Down Expand Up @@ -197,4 +194,4 @@ export default (async function runDoctor(
};

printFixOptions({onKeyPress});
});
}) as CommandFunction;
3 changes: 2 additions & 1 deletion packages/cli/src/commands/doctor/healthchecks/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import androidNDK from './androidNDK';
import xcode from './xcode';
import cocoaPods from './cocoaPods';
import iosDeploy from './iosDeploy';
import {Healthchecks} from '../types';

export const HEALTHCHECK_TYPES = {
ERROR: 'ERROR',
Expand All @@ -18,7 +19,7 @@ type Options = {
contributor: boolean | void;
};

export const getHealthchecks = ({contributor}: Options) => ({
export const getHealthchecks = ({contributor}: Options): Healthchecks => ({
common: {
label: 'Common',
healthchecks: [
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
// @flow
import doctor from './doctor';

export default {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,46 +1,47 @@
// @flow
import chalk from 'chalk';
import ora from 'ora';
import ora, {Ora} from 'ora';
import {logger} from '@react-native-community/cli-tools';
// $FlowFixMe - converted to TS
import {HEALTHCHECK_TYPES} from './healthchecks';
import type {EnvironmentInfo} from './types';
import {EnvironmentInfo, HealthCheckCategoryResult} from './types';

const AUTOMATIC_FIX_LEVELS = {
ALL_ISSUES: 'ALL_ISSUES',
ERRORS: 'ERRORS',
WARNINGS: 'WARNINGS',
};
export enum AUTOMATIC_FIX_LEVELS {
Copy link
Member

Choose a reason for hiding this comment

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

so good

ALL_ISSUES = 'ALL_ISSUES',
ERRORS = 'ERRORS',
WARNINGS = 'WARNINGS',
}

export {AUTOMATIC_FIX_LEVELS};
export default async ({
interface RunAutomaticFixArgs {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Cool, thanks for replying!

healthchecks: HealthCheckCategoryResult[];
automaticFixLevel: AUTOMATIC_FIX_LEVELS;
stats: {
errors: number;
warnings: number;
};
loader: Ora;
environmentInfo: EnvironmentInfo;
}

export default async function({
healthchecks,
automaticFixLevel,
stats,
loader,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

environmentInfo,
}: {
healthchecks: any,
automaticFixLevel: $Values<typeof AUTOMATIC_FIX_LEVELS>,
stats: {errors: any, warnings: any},
loader: typeof ora,
environmentInfo: EnvironmentInfo,
}) => {
}: RunAutomaticFixArgs) {
// Remove the fix options from screen
// $FlowFixMe
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

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

process.stdout.moveCursor(0, -6);
// $FlowFixMe
// @ts-ignore
process.stdout.clearScreenDown();

const totalIssuesBasedOnFixLevel = {
const totalIssuesBasedOnFixLevel: {[x in AUTOMATIC_FIX_LEVELS]: number} = {
[AUTOMATIC_FIX_LEVELS.ALL_ISSUES]: stats.errors + stats.warnings,
[AUTOMATIC_FIX_LEVELS.ERRORS]: stats.errors,
[AUTOMATIC_FIX_LEVELS.WARNINGS]: stats.warnings,
};
const issuesCount = totalIssuesBasedOnFixLevel[automaticFixLevel];

logger.log(
`\nAttempting to fix ${chalk.bold(issuesCount)} issue${
`\nAttempting to fix ${chalk.bold(issuesCount.toString())} issue${
issuesCount > 1 ? 's' : ''
}...`,
);
Expand All @@ -64,6 +65,8 @@ export default async ({
healthcheck.type === HEALTHCHECK_TYPES.WARNING
);
}

return;
});

if (!healthchecksToRun.length) {
Expand All @@ -88,4 +91,4 @@ export default async ({
}
}
}
};
}
79 changes: 0 additions & 79 deletions packages/cli/src/commands/doctor/types.js

This file was deleted.

Loading