[doctor] check for @types/react-native on 48+ (and other things you s…

…houldn't install directly) (#4731)

* [doctor] check for @types/react-native on 48+

* also check for npm/ yarn installs

* add pnpm to direct install check

packages/expo-doctor/src/checks/DirectPackageInstallCheck.ts

@@ -0,0 +1,83 @@
+import { DoctorMultiCheck, DoctorMultiCheckItemBase } from './DoctorMultiCheck';
+import { DoctorCheckParams, DoctorCheckResult } from './checks.types';
+
+export type DirectPackageInstallCheckItem = {
+  packageName: string;
+} & DoctorMultiCheckItemBase;
+
+const baseCheckItem = {
+  getMessage: (packageName: string) =>
+    `The package "${packageName}" should not be installed directly in your project. It is a dependency of other Expo packages, which will install it automatically as needed.`,
+  sdkVersionRange: '*',
+};
+
+const shouldBeInstalledGloballyItem = {
+  getMessage: (packageName: string) =>
+    `The package "${packageName}" should not be installed directly in your project. It may conflict with the globally-installed version.`,
+  sdkVersionRange: '*',
+};
+
+export const directPackageInstallCheckItems: DirectPackageInstallCheckItem[] = [
+  {
+    packageName: 'expo-modules-core',
+    ...baseCheckItem,
+  },
+  {
+    packageName: 'expo-modules-autolinking',
+    ...baseCheckItem,
+  },
+  {
+    packageName: 'expo-dev-launcher',
+    ...baseCheckItem,
+  },
+  {
+    packageName: 'expo-dev-menu',
+    ...baseCheckItem,
+  },
+  {
+    packageName: 'npm',
+    ...shouldBeInstalledGloballyItem,
+  },
+  {
+    packageName: 'yarn',
+    ...shouldBeInstalledGloballyItem,
+  },
+  {
+    packageName: 'pnpm',
+    ...shouldBeInstalledGloballyItem,
+  },
+  {
+    packageName: '@types/react-native',
+    getMessage: () =>
+      `The package  "@types/react-native" should not be installed directly in your project, as types are included with the "react-native" package.`,
+    sdkVersionRange: '>=48.0.0',
+  },
+];
+
+export class DirectPackageInstallCheck extends DoctorMultiCheck<DirectPackageInstallCheckItem> {
+  description = 'Check dependencies for packages that should not be installed directly';
+
+  sdkVersionRange = '*';
+
+  checkItems = directPackageInstallCheckItems;
+
+  protected async runAsyncInner(
+    { pkg }: DoctorCheckParams,
+    checkItems: DirectPackageInstallCheckItem[]
+  ): Promise<DoctorCheckResult> {
+    const issues: string[] = [];
+
+    // ** check for dependencies that should only be transitive **
+    checkItems.forEach(checkItem => {
+      const packageName = checkItem.packageName;
+      if (pkg.dependencies?.[packageName] || pkg.devDependencies?.[packageName]) {
+        issues.push(checkItem.getMessage(packageName));
+      }
+    });
+
+    return {
+      isSuccessful: issues.length === 0,
+      issues,
+    };
+  }
+}

packages/expo-doctor/src/checks/DoctorMultiCheck.ts

@@ -0,0 +1,34 @@
+import semver from 'semver';
+
+import { DoctorCheck, DoctorCheckParams, DoctorCheckResult } from './checks.types';
+
+export interface DoctorMultiCheckItemBase {
+  getMessage: ((packageName: string) => string) | (() => string);
+  sdkVersionRange: string;
+}
+
+//
+export abstract class DoctorMultiCheck<TCheckItem extends DoctorMultiCheckItemBase>
+  implements DoctorCheck {
+  abstract readonly checkItems: TCheckItem[];
+
+  abstract description: string;
+
+  // will be the most permissive semver for all check items
+  abstract sdkVersionRange: string;
+
+  protected abstract runAsyncInner(
+    params: DoctorCheckParams,
+    checkItems: TCheckItem[]
+  ): Promise<DoctorCheckResult>;
+
+  async runAsync(params: DoctorCheckParams): Promise<DoctorCheckResult> {
+    const filteredCheckItems = this.checkItems.filter(
+      check =>
+        params.exp.sdkVersion === 'UNVERSIONED' ||
+        semver.satisfies(params.exp.sdkVersion!, check.sdkVersionRange)
+    );
+
+    return this.runAsyncInner(params, filteredCheckItems);
+  }
+}

packages/expo-doctor/src/checks/PackageJsonCheck.ts

@@ -1,25 +1,8 @@
-import { PackageJSONConfig } from '@expo/config';
 import fs from 'fs';
 import path from 'path';
 
 import { DoctorCheck, DoctorCheckParams, DoctorCheckResult } from './checks.types';
 
-/**
- * Checks if a package should not be installed directly in a project.
- * @param pkg package.json config
- * @param packageName name of package to check for
- * @returns string if package should not be installed directly, null otherwise
- */
-function checkForInvalidDirectInstallPackage(
-  pkg: PackageJSONConfig,
-  packageName: string
-): string | null {
-  if (pkg.dependencies?.[packageName] || pkg.devDependencies?.[packageName]) {
-    return `The package "${packageName}" should not be installed directly in your project. It is a dependency of other Expo packages, which will install it automatically as needed.`;
-  }
-  return null;
-}
-
 export class PackageJsonCheck implements DoctorCheck {
   description = 'Check package.json for common issues';
 
@@ -47,16 +30,6 @@ export class PackageJsonCheck implements DoctorCheck {
       );
     }
 
-    // ** check for dependencies that should only be transitive **
-    ['expo-modules-core', 'expo-modules-autolinking', 'expo-dev-launcher', 'expo-dev-menu'].forEach(
-      packageName => {
-        const result = checkForInvalidDirectInstallPackage(pkg, packageName);
-        if (result) {
-          issues.push(result);
-        }
-      }
-    );
-
     // ** check for conflicts between package name and installed packages **
 
     const installedPackages = Object.keys(pkg.dependencies ?? {}).concat(

packages/expo-doctor/src/checks/__tests__/DirectPackageInstallCheck.test.ts

@@ -0,0 +1,79 @@
+import { vol } from 'memfs';
+
+import {
+  DirectPackageInstallCheck,
+  directPackageInstallCheckItems,
+} from '../DirectPackageInstallCheck';
+
+jest.mock('fs');
+
+const projectRoot = '/tmp/project';
+
+// required by runAsync
+const additionalProjectProps = {
+  exp: {
+    name: 'name',
+    slug: 'slug',
+    sdkVersion: '48.0.0',
+  },
+  projectRoot,
+};
+
+describe('runAsync', () => {
+  beforeEach(() => {
+    vol.fromJSON({
+      [projectRoot + '/node_modules/.bin/expo']: 'test',
+    });
+  });
+
+  it('returns result with isSuccessful = true if empty dependencies, devDependencies, scripts', async () => {
+    const check = new DirectPackageInstallCheck();
+    const result = await check.runAsync({
+      pkg: { name: 'name', version: '1.0.0' },
+      ...additionalProjectProps,
+    });
+    expect(result.isSuccessful).toBeTruthy();
+  });
+
+  it(`returns result with isSuccessful = true if package.json contains a dependency that is on list, but SDK requirement is not met`, async () => {
+    const check = new DirectPackageInstallCheck();
+    const result = await check.runAsync({
+      pkg: { name: 'name', version: '1.0.0', dependencies: { '@types/react-native': '17.0.1' } },
+      ...additionalProjectProps,
+      exp: {
+        name: 'name',
+        slug: 'slug',
+        sdkVersion: '47.0.0',
+      },
+    });
+    expect(result.isSuccessful).toBeTruthy();
+  });
+
+  const dependencyLocations = ['dependencies', 'devDependencies'];
+
+  dependencyLocations.forEach(dependencyLocation => {
+    it(`returns result with isSuccessful = true if ${dependencyLocation} does not contain a dependency that should only be installed by another Expo package`, async () => {
+      const check = new DirectPackageInstallCheck();
+      const result = await check.runAsync({
+        pkg: { name: 'name', version: '1.0.0', [dependencyLocation]: { somethingjs: '17.0.1' } },
+        ...additionalProjectProps,
+      });
+      expect(result.isSuccessful).toBeTruthy();
+    });
+
+    directPackageInstallCheckItems.forEach(transitiveOnlyDependency => {
+      it(`returns result with isSuccessful = false if ${dependencyLocation} contains ${transitiveOnlyDependency.packageName}`, async () => {
+        const check = new DirectPackageInstallCheck();
+        const result = await check.runAsync({
+          pkg: {
+            name: 'name',
+            version: '1.0.0',
+            [dependencyLocation]: { [transitiveOnlyDependency.packageName]: '1.0.0' },
+          },
+          ...additionalProjectProps,
+        });
+        expect(result.isSuccessful).toBeFalsy();
+      });
+    });
+  });
+});

packages/expo-doctor/src/checks/__tests__/PackageJsonCheck.test.ts

@@ -73,43 +73,6 @@ describe('runAsync', () => {
     expect(result.isSuccessful).toBeFalsy();
   });
 
-  // transitive-only dependencies sub-check
-
-  const dependencyLocations = ['dependencies', 'devDependencies'];
-
-  const transitiveOnlyDependencies = [
-    'expo-modules-core',
-    'expo-modules-autolinking',
-    'expo-dev-launcher',
-    'expo-dev-menu',
-  ];
-
-  dependencyLocations.forEach(dependencyLocation => {
-    it(`returns result with isSuccessful = true if ${dependencyLocation} does not contain a dependency that should only be installed by another Expo package`, async () => {
-      const check = new PackageJsonCheck();
-      const result = await check.runAsync({
-        pkg: { name: 'name', version: '1.0.0', [dependencyLocation]: { somethingjs: '17.0.1' } },
-        ...additionalProjectProps,
-      });
-      expect(result.isSuccessful).toBeTruthy();
-    });
-
-    transitiveOnlyDependencies.forEach(transitiveOnlyDependency => {
-      it(`returns result with isSuccessful = false if ${dependencyLocation} contains ${transitiveOnlyDependency}`, async () => {
-        const check = new PackageJsonCheck();
-        const result = await check.runAsync({
-          pkg: {
-            name: 'name',
-            version: '1.0.0',
-            [dependencyLocation]: { [transitiveOnlyDependency]: '1.0.0' },
-          },
-          ...additionalProjectProps,
-        });
-        expect(result.isSuccessful).toBeFalsy();
-      });
-    });
-  });
-
   // package name conflicts with installed packages sub-check
   it('returns result with isSuccessful = true if package name does not match dependency', async () => {
     const check = new PackageJsonCheck();

packages/expo-doctor/src/doctor.ts

@@ -2,6 +2,7 @@ import { getConfig } from '@expo/config';
 import chalk from 'chalk';
 import semver from 'semver';
 
+import { DirectPackageInstallCheck } from './checks/DirectPackageInstallCheck';
 import { ExpoConfigSchemaCheck } from './checks/ExpoConfigSchemaCheck';
 import { GlobalPackageInstalledCheck } from './checks/GlobalPackageInstalledCheck';
 import { GlobalPrereqsVersionCheck } from './checks/GlobalPrereqsVersionCheck';
@@ -139,6 +140,7 @@ export async function actionAsync(projectRoot: string) {
     new SupportPackageVersionCheck(),
     new InstalledDependencyVersionCheck(),
     new ExpoConfigSchemaCheck(),
+    new DirectPackageInstallCheck(),
     new PackageJsonCheck(),
     new ProjectSetupCheck(),
   ];