TypeScript validation during Metro bundling (#392)

* Update typescript-service:
- Separate Project methods into "find" and "open", allowing caller to decide on the appropriate error-handling logic.
- Add filtering when validating Project files. Only include ts[x] unless checkJs is enabled.
- Make cache methods return a true/false result, moving error-handling up and out to the caller (Project).
- Stop converting to lowercase during path normalization. This confuses typescript.

* Add typescript validation to CLI rnx-bundle call.

* Don't check JS files when bundling test-app, as this would expose all of react-native JS files. They aren't typesafe and fail the check.

* Fix clean command (build) to clear out the lage cache, ensuring that everything is rebuilt and retested.

* Change files

* change file

* PR feedback

change/@rnx-kit-cli-66065206-bef4-4776-a573-ce6a1e2a137a.json

@@ -0,0 +1,7 @@
+{
+  "type": "minor",
+  "comment": "Add typescript validation to CLI rnx-bundle call.",
+  "packageName": "@rnx-kit/cli",
+  "email": "afoxman@microsoft.com",
+  "dependentChangeType": "patch"
+}

change/@rnx-kit-typescript-service-356b56b5-4645-4dee-a999-f1fb2c6a961b.json

@@ -0,0 +1,7 @@
+{
+  "type": "minor",
+  "comment": "Separate Project methods into \"find\" and \"open\", allowing caller to decide on the appropriate error-handling logic. Add filtering when validating Project files -- 0nly include ts[x] unless checkJs is enabled. Make cache methods return a true/false result, moving error-handling up and out to the caller (Project). Stop converting to lowercase during path normalization, as this confuses typescript.",
+  "packageName": "@rnx-kit/typescript-service",
+  "email": "afoxman@microsoft.com",
+  "dependentChangeType": "patch"
+}

package.json

@@ -23,7 +23,7 @@
     "change": "beachball change",
     "check": "beachball check --branch origin/main --changehint \"Run 'yarn run change' from root of repo to generate a change file.\"",
     "ci": "yarn --cache-folder .yarn-offline-mirror --prefer-offline --frozen-lockfile --non-interactive",
-    "clean": "lage clean --log-level warn --grouped",
+    "clean": "lage cache --clear && lage clean --log-level warn --grouped",
     "depcheck": "lage depcheck --log-level warn --grouped",
     "format": "lage format --log-level warn --grouped",
     "lint": "lage lint --verbose --grouped --no-cache",

packages/cli/package.json

@@ -27,6 +27,7 @@
     "@rnx-kit/metro-serializer-esbuild": "^0.0.10",
     "@rnx-kit/metro-service": "1.0.2",
     "@rnx-kit/third-party-notices": "1.0.1",
+    "@rnx-kit/typescript-service": "1.0.2",
     "chalk": "^4.1.0"
   },
   "devDependencies": {

packages/cli/src/bundle.ts

@@ -7,10 +7,12 @@ import {
   getKitConfig,
 } from "@rnx-kit/config";
 import { bundle, BundleArgs, loadMetroConfig } from "@rnx-kit/metro-service";
+import { Service } from "@rnx-kit/typescript-service";
 import chalk from "chalk";
 import fs from "fs";
 import path from "path";
 import { customizeMetroConfig, validateMetroConfig } from "./metro-config";
+import type { TSProjectInfo } from "./types";
 
 type CLIBundleOptions = {
   id?: string;
@@ -118,8 +120,13 @@ export async function rnxBundle(
     );
   }
 
+  //  create a typescript service
+  const tsservice = new Service();
+
   //  create a bundle for each target platform
   for (const targetPlatform of targetPlatforms) {
+    console.log(`Bundling ${targetPlatform}...`);
+
     //  unpack the platform-specific bundle definition
     const platformDefinition = getBundlePlatformDefinition(
       definition,
@@ -134,10 +141,10 @@ export async function rnxBundle(
       sourceMapPath,
       sourceMapSourceRootPath,
     } = platformDefinition;
-
     const {
       detectCyclicDependencies,
       detectDuplicateDependencies,
+      typescriptValidation,
       experimental_treeShake,
     } = platformDefinition;
 
@@ -169,10 +176,28 @@ export async function rnxBundle(
       sourceMapPath = path.join(distPath, sourceMapPath);
     }
 
+    let tsprojectInfo: TSProjectInfo | undefined;
+    if (typescriptValidation) {
+      const configFileName = tsservice.findProject(entryPath, "tsconfig.json");
+      if (!configFileName) {
+        console.warn(
+          "skipping TypeScript validation -- cannot find tsconfig.json for entry file %o",
+          entryPath
+        );
+      } else {
+        tsprojectInfo = {
+          platform: targetPlatform,
+          service: tsservice,
+          configFileName,
+        };
+      }
+    }
+
     customizeMetroConfig(
       metroConfig,
       detectCyclicDependencies,
       detectDuplicateDependencies,
+      tsprojectInfo,
       experimental_treeShake
     );
 
@@ -182,7 +207,6 @@ export async function rnxBundle(
     assetsPath && ensureDirectoryExists(assetsPath);
 
     //  create the bundle
-    console.log(`Bundling ${targetPlatform}...`);
     await bundle(
       {
         assetsDest: assetsPath,

packages/cli/src/metro-config.ts

@@ -6,9 +6,11 @@ import {
   DuplicateDependencies,
   Options as DuplicateDetectorOptions,
 } from "@rnx-kit/metro-plugin-duplicates-checker";
-import { MetroSerializer, MetroPlugin } from "@rnx-kit/metro-serializer";
+import { MetroPlugin, MetroSerializer } from "@rnx-kit/metro-serializer";
 import { MetroSerializer as MetroSerializerEsbuild } from "@rnx-kit/metro-serializer-esbuild";
+import type { DeltaResult, Graph } from "metro";
 import type { ConfigT, InputConfigT, SerializerConfigT } from "metro-config";
+import type { TSProjectInfo } from "./types";
 
 function reportError(prop: string) {
   console.error(
@@ -29,11 +31,16 @@ export function validateMetroConfig(metroConfig: ConfigT): boolean {
   return true;
 }
 
+const emptySerializerHook = (_graph: Graph, _delta: DeltaResult): void => {
+  // nop
+};
+
 //  Customize the Metro configuration
 export function customizeMetroConfig(
   metroConfigReadonly: InputConfigT,
   detectCyclicDependencies: boolean | CyclicDetectorOptions,
   detectDuplicateDependencies: boolean | DuplicateDetectorOptions,
+  tsprojectInfo: TSProjectInfo | undefined,
   experimental_treeShake: boolean
 ): void {
   //  We will be making changes to the Metro configuration. Coerce from a
@@ -71,4 +78,37 @@ export function customizeMetroConfig(
   } else {
     delete metroConfig.serializer.customSerializer;
   }
+
+  metroConfig.serializer.experimentalSerializerHook = emptySerializerHook;
+  if (tsprojectInfo) {
+    const { service, configFileName } = tsprojectInfo;
+
+    const tsproject = service.openProject(configFileName);
+    if (tsproject) {
+      //  start with an empty project, ignoring the file graph provided by tsconfig.json
+      tsproject.removeAllFiles();
+
+      const hook = (_graph: Graph, delta: DeltaResult): void => {
+        //  keep the TS project in sync with Metro's file graph
+        if (delta.reset) {
+          tsproject.removeAllFiles();
+        }
+
+        for (const module of delta.added.values()) {
+          tsproject.addFile(module.path);
+        }
+        for (const module of delta.modified.values()) {
+          tsproject.updateFile(module.path);
+        }
+        for (const module of delta.modified.values()) {
+          tsproject.removeFile(module.path);
+        }
+
+        //  validate the project, printing errors to the console
+        tsproject.validate();
+      };
+
+      metroConfig.serializer.experimentalSerializerHook = hook;
+    }
+  }
 }

packages/cli/src/types.ts

@@ -0,0 +1,7 @@
+import type { Service } from "@rnx-kit/typescript-service";
+
+export type TSProjectInfo = {
+  platform: string;
+  service: Service;
+  configFileName: string;
+};

packages/test-app/tsconfig.json

@@ -1,4 +1,7 @@
 {
   "extends": "rnx-kit-scripts/tsconfig-shared.json",
+  "compilerOptions": {
+    "checkJs": false
+  },
   "include": ["src"]
 }

packages/typescript-service/README.md

@@ -17,10 +17,11 @@ The `Service` is used to open a `Project`.
 const service = new Service();
 
 // Find a project file and open it
-const project = service.openProject("./", "tsconfig.json");
+const configFileName = service.findProject("./", "tsconfig.json");
+const project = service.openProject(configFileName);
 
 // Open a project file directly
-const project = service.openProjectByFile(path.resolve("./tsconfig.json"));
+const project = service.openProject(path.resolve("./tsconfig.json"));
 ```
 
 ## Project

packages/typescript-service/package.json

@@ -26,7 +26,6 @@
     "@types/node": "^14.15.0",
     "@rnx-kit/babel-preset-jest-typescript": "*",
     "jest-extended": "^0.11.5",
-    "jest-mock-console": "^1.1.0",
     "rnx-kit-scripts": "*"
   },
   "eslintConfig": {

packages/typescript-service/src/cache.ts

@@ -9,23 +9,41 @@ export class ProjectFileCache {
     fileNames.forEach((fileName) => this.add(fileName));
   }
 
-  add(fileName: string): void {
+  has(fileName: string): boolean {
     const normalized = normalizePath(fileName);
+    return this.files.has(normalized);
+  }
+
+  add(fileName: string): boolean {
+    const normalized = normalizePath(fileName);
+    if (this.files.has(normalized)) {
+      return false;
+    }
     this.files.set(normalized, new VersionedSnapshot(normalized));
+    return true;
   }
 
-  update(fileName: string, snapshot?: ts.IScriptSnapshot): void {
+  update(fileName: string, snapshot?: ts.IScriptSnapshot): boolean {
     const normalized = normalizePath(fileName);
-    const file = this.files.get(normalized);
-    if (!file) {
-      throw new Error(`Cannot update unknown project file ${fileName}`);
+    if (!this.files.has(normalized)) {
+      return false;
     }
-    file.update(snapshot);
+    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- Map has the element, and we only store non-null objects
+    this.files.get(normalized)!.update(snapshot);
+    return true;
   }
 
-  delete(fileName: string): void {
+  delete(fileName: string): boolean {
     const normalized = normalizePath(fileName);
+    if (!this.files.has(normalized)) {
+      return false;
+    }
     this.files.delete(normalized);
+    return true;
+  }
+
+  deleteAll(): void {
+    this.files.clear();
   }
 
   getFileNames(): string[] {
@@ -56,6 +74,7 @@ export class ExternalFileCache {
       this.files.set(normalized, file);
     }
 
-    return file.getSnapshot();
+    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- Map has the element, and we only store non-null objects
+    return this.files.get(normalized)!.getSnapshot();
   }
 }

packages/typescript-service/src/project.ts

@@ -1,8 +1,8 @@
 import ts from "typescript";
-import { ProjectFileCache, ExternalFileCache } from "./cache";
-import { Resolvers } from "./resolve";
+import { ExternalFileCache, ProjectFileCache } from "./cache";
 import { ProjectConfig } from "./config";
 import { DiagnosticWriter } from "./diagnostics";
+import { Resolvers } from "./resolve";
 import { isNonEmptyArray } from "./util";
 
 export class Project {
@@ -134,24 +134,43 @@ export class Project {
   }
 
   validate(): boolean {
+    //  filter down the list of files to be checked
+    const matcher = this.projectConfig.options.checkJs
+      ? /[.][jt]sx?$/
+      : /[.]tsx?$/;
+    const files = this.projectFiles
+      .getFileNames()
+      .filter((f) => f.match(matcher));
+
+    //  check each file
     let result = true;
-    this.projectFiles.getFileNames().forEach((f) => {
-      const fileResult = this.validateFile(f);
+    for (const file of files) {
+      //  always validate the file, even if others have failed
+      const fileResult = this.validateFile(file);
+      //  combine this file's result with the aggregate result
       result = result && fileResult;
-    });
+    }
     return result;
   }
 
-  addFile(fileName: string): void {
-    this.projectFiles.add(fileName);
+  hasFile(fileName: string): boolean {
+    return this.projectFiles.has(fileName);
+  }
+
+  addFile(fileName: string): boolean {
+    return this.projectFiles.add(fileName);
+  }
+
+  updateFile(fileName: string, snapshot?: ts.IScriptSnapshot): boolean {
+    return this.projectFiles.update(fileName, snapshot);
   }
 
-  updateFile(fileName: string, snapshot?: ts.IScriptSnapshot): void {
-    this.projectFiles.update(fileName, snapshot);
+  removeFile(fileName: string): boolean {
+    return this.projectFiles.delete(fileName);
   }
 
-  removeFile(fileName: string): void {
-    this.projectFiles.delete(fileName);
+  removeAllFiles(): void {
+    this.projectFiles.deleteAll();
   }
 
   dispose(): void {

packages/typescript-service/src/service.ts

@@ -15,24 +15,14 @@ export class Service {
     this.projectConfigLoader = new ProjectConfigLoader(this.diagnosticWriter);
   }
 
-  openProject(
+  findProject(
     searchPath: string,
     fileName = "tsconfig.json"
-  ): Project | undefined {
-    const configFileName = this.projectConfigLoader.find(searchPath, fileName);
-    if (!configFileName) {
-      console.error(
-        "Cannot find project configuration file %o under search-path %o",
-        fileName,
-        searchPath
-      );
-      return undefined;
-    }
-
-    return this.openProjectByFile(configFileName);
+  ): string | undefined {
+    return this.projectConfigLoader.find(searchPath, fileName);
   }
 
-  openProjectByFile(configFileName: string): Project {
+  openProject(configFileName: string): Project {
     const config = this.projectConfigLoader.load(configFileName);
     const resolvers = createResolvers(config.options);
     return new Project(

packages/typescript-service/src/util.ts

@@ -13,6 +13,5 @@ export function isNonEmptyArray(a: unknown): a is Array<unknown> {
 }
 
 export function normalizePath(p: string): string {
-  const fixedPathSeparators = p.replace(/\\/g, "/");
-  return getCanonicalFileName(fixedPathSeparators);
+  return p.replace(/\\/g, "/");
 }