refactor(@ngtools/webpack): use type guard based narrowing with TS AST

clydin · clydin · commit e852b62fcedb · 2020-08-03T15:39:39.000-04:00
By leveraging TypeScript's AST type guards, function parameter assumptions and casting can be removed.  Many of these cases caused errors when enabling TypeScript's strict option. This is preliminary work to support enabling full TypeScript strict mode within the project.
diff --git a/packages/ngtools/webpack/src/entry_resolver.ts b/packages/ngtools/webpack/src/entry_resolver.ts
@@ -8,16 +8,16 @@
 
 import { Path, join } from '@angular-devkit/core';
 import * as ts from 'typescript';
-import { TypeScriptFileRefactor } from './refactor';
+import { TypeScriptFileRefactor, findAstNodes } from './refactor';
 
 
 function _recursiveSymbolExportLookup(refactor: TypeScriptFileRefactor,
                                       symbolName: string,
                                       host: ts.CompilerHost,
                                       program: ts.Program): string | null {
   // Check this file.
-  const hasSymbol = refactor.findAstNodes(null, ts.SyntaxKind.ClassDeclaration)
-    .some((cd: ts.ClassDeclaration) => {
+  const hasSymbol = findAstNodes(null, refactor.sourceFile, ts.isClassDeclaration)
+    .some((cd) => {
       return cd.name != undefined && cd.name.text == symbolName;
     });
   if (hasSymbol) {
@@ -69,8 +69,8 @@ function _recursiveSymbolExportLookup(refactor: TypeScriptFileRefactor,
 
         // Create the source and verify that the symbol is at least a class.
         const source = new TypeScriptFileRefactor(module, host, program);
-        const hasSymbol = source.findAstNodes(null, ts.SyntaxKind.ClassDeclaration)
-          .some((cd: ts.ClassDeclaration) => {
+        const hasSymbol = findAstNodes(null, source.sourceFile, ts.isClassDeclaration)
+          .some((cd) => {
             return cd.name != undefined && cd.name.text == symbolName;
           });
 
diff --git a/packages/ngtools/webpack/src/lazy_routes.ts b/packages/ngtools/webpack/src/lazy_routes.ts
@@ -57,24 +57,21 @@ export function findLazyRoutes(
   }
   const sf: ts.SourceFile = sourceFile;
 
-  return findAstNodes(null, sourceFile, ts.SyntaxKind.ObjectLiteralExpression, true)
+  return findAstNodes(null, sourceFile, ts.isObjectLiteralExpression, true)
     // Get all their property assignments.
-    .map((node: ts.ObjectLiteralExpression) => {
-      return findAstNodes(node, sf, ts.SyntaxKind.PropertyAssignment, false);
-    })
+    .map((node) => findAstNodes(node, sf, ts.isPropertyAssignment, false))
     // Take all `loadChildren` elements.
-    .reduce((acc: ts.PropertyAssignment[], props: ts.PropertyAssignment[]) => {
+    .reduce((acc, props) => {
       return acc.concat(props.filter(literal => {
         return _getContentOfKeyLiteral(sf, literal.name) == 'loadChildren';
       }));
     }, [])
     // Get only string values.
-    .filter((node: ts.PropertyAssignment) => node.initializer.kind == ts.SyntaxKind.StringLiteral)
-    // Get the string value.
-    .map((node: ts.PropertyAssignment) => (node.initializer as ts.StringLiteral).text)
+    .map((node) => node.initializer)
+    .filter(ts.isStringLiteral)
     // Map those to either [path, absoluteModulePath], or [path, null] if the module pointing to
     // does not exist.
-    .map((routePath: string) => {
+    .map(({ text: routePath }) => {
       const moduleName = routePath.split('#')[0];
       const compOptions = (program && program.getCompilerOptions()) || compilerOptions || {};
       const resolvedModuleName: ts.ResolvedModuleWithFailedLookupLocations = moduleName[0] == '.'
@@ -87,11 +84,11 @@ export function findLazyRoutes(
           && host.fileExists(resolvedModuleName.resolvedModule.resolvedFileName)) {
         return [routePath, resolvedModuleName.resolvedModule.resolvedFileName];
       } else {
-        return [routePath, null];
+        return [routePath, null] as [string, null];
       }
     })
     // Reduce to the LazyRouteMap map.
-    .reduce((acc: LazyRouteMap, [routePath, resolvedModuleName]: [string, string | null]) => {
+    .reduce((acc: LazyRouteMap, [routePath, resolvedModuleName]) => {
       if (resolvedModuleName) {
         acc[routePath] = resolvedModuleName;
       }
diff --git a/packages/ngtools/webpack/src/refactor.ts b/packages/ngtools/webpack/src/refactor.ts
@@ -19,11 +19,35 @@ import { forwardSlashPath } from './utils';
  * @param max The maximum number of items to return.
  * @return all nodes of kind, or [] if none is found
  */
-// TODO: replace this with collectDeepNodes and add limits to collectDeepNodes
-export function findAstNodes<T extends ts.Node>(
+export function findAstNodes(
   node: ts.Node | null,
   sourceFile: ts.SourceFile,
   kind: ts.SyntaxKind,
+  recursive?: boolean,
+  max?: number,
+): ts.Node[];
+
+/**
+ * Find all nodes from the AST in the subtree of node of SyntaxKind kind.
+ * @param node The root node to check, or null if the whole tree should be searched.
+ * @param sourceFile The source file where the node is.
+ * @param guard
+ * @param recursive Whether to go in matched nodes to keep matching.
+ * @param max The maximum number of items to return.
+ * @return all nodes of kind, or [] if none is found
+ */
+export function findAstNodes<T extends ts.Node>(
+  node: ts.Node | null,
+  sourceFile: ts.SourceFile,
+  guard: (node: ts.Node) => node is T,
+  recursive?: boolean,
+  max?: number,
+): T[];
+
+export function findAstNodes<T extends ts.Node>(
+  node: ts.Node | null,
+  sourceFile: ts.SourceFile,
+  kindOrGuard: ts.SyntaxKind | ((node: ts.Node) => node is T),
   recursive = false,
   max = Infinity,
 ): T[] {
@@ -35,23 +59,28 @@ export function findAstNodes<T extends ts.Node>(
     node = sourceFile;
   }
 
+  const test =
+    typeof kindOrGuard === 'function'
+      ? kindOrGuard
+      : (node: ts.Node): node is T => node.kind === kindOrGuard;
+
   const arr: T[] = [];
-  if (node.kind === kind) {
+  if (test(node)) {
     // If we're not recursively looking for children, stop here.
     if (!recursive) {
       return [node as T];
     }
 
-    arr.push(node as T);
+    arr.push(node);
     max--;
   }
 
   if (max > 0) {
     for (const child of node.getChildren(sourceFile)) {
-      findAstNodes(child, sourceFile, kind, recursive, max)
-        .forEach((node: ts.Node) => {
+      findAstNodes(child, sourceFile, test, recursive, max)
+        .forEach((node) => {
           if (max > 0) {
-            arr.push(node as T);
+            arr.push(node);
           }
           max--;
         });
diff --git a/packages/ngtools/webpack/src/transformers/find_resources.ts b/packages/ngtools/webpack/src/transformers/find_resources.ts
@@ -27,7 +27,7 @@ export function findResources(sourceFile: ts.SourceFile): string[] {
 
     ts.visitNodes(
       (args[0] as ts.ObjectLiteralExpression).properties,
-      (node: ts.ObjectLiteralElementLike) => {
+      (node) => {
         if (!ts.isPropertyAssignment(node) || ts.isComputedPropertyName(node.name)) {
           return node;
         }
@@ -47,7 +47,7 @@ export function findResources(sourceFile: ts.SourceFile): string[] {
               return node;
             }
 
-            ts.visitNodes(node.initializer.elements, (node: ts.Expression) => {
+            ts.visitNodes(node.initializer.elements, (node) => {
               const url = getResourceUrl(node);
 
               if (url) {
diff --git a/packages/ngtools/webpack/src/transformers/insert_import.ts b/packages/ngtools/webpack/src/transformers/insert_import.ts
@@ -69,12 +69,13 @@ export function insertImport(
   // Find all imports.
   const allImports = collectDeepNodes(sourceFile, ts.SyntaxKind.ImportDeclaration);
   const maybeImports = allImports
-    .filter((node: ts.ImportDeclaration) => {
+    .filter(ts.isImportDeclaration)
+    .filter((node) => {
       // Filter all imports that do not match the modulePath.
       return node.moduleSpecifier.kind == ts.SyntaxKind.StringLiteral
         && (node.moduleSpecifier as ts.StringLiteral).text == modulePath;
     })
-    .filter((node: ts.ImportDeclaration) => {
+    .filter((node) => {
       // Filter out import statements that are either `import 'XYZ'` or `import * as X from 'XYZ'`.
       const clause = node.importClause as ts.ImportClause;
       if (!clause || clause.name || !clause.namedBindings) {
@@ -83,7 +84,7 @@ export function insertImport(
 
       return clause.namedBindings.kind == ts.SyntaxKind.NamedImports;
     })
-    .map((node: ts.ImportDeclaration) => {
+    .map((node) => {
       // Return the `{ ... }` list of the named import.
       return (node.importClause as ts.ImportClause).namedBindings as ts.NamedImports;
     });
diff --git a/packages/ngtools/webpack/src/transformers/replace_resources.ts b/packages/ngtools/webpack/src/transformers/replace_resources.ts
@@ -29,8 +29,10 @@ export function replaceResources(
 
     const visitNode: ts.Visitor = (node: ts.Node) => {
       if (ts.isClassDeclaration(node)) {
-        const decorators = ts.visitNodes(node.decorators, (node: ts.Decorator) =>
-          visitDecorator(context, node, typeChecker, directTemplateLoading),
+        const decorators = ts.visitNodes(node.decorators, (node) =>
+          ts.isDecorator(node)
+            ? visitDecorator(context, node, typeChecker, directTemplateLoading)
+            : node,
         );
 
         return ts.updateClassDeclaration(
@@ -82,8 +84,10 @@ function visitDecorator(
   const styleReplacements: ts.Expression[] = [];
 
   // visit all properties
-  let properties = ts.visitNodes(objectExpression.properties, (node: ts.ObjectLiteralElementLike) =>
-    visitComponentMetadata(context, node, styleReplacements, directTemplateLoading),
+  let properties = ts.visitNodes(objectExpression.properties, (node) =>
+    ts.isObjectLiteralElementLike(node)
+      ? visitComponentMetadata(context, node, styleReplacements, directTemplateLoading)
+      : node,
   );
 
   // replace properties with updated properties
@@ -137,7 +141,7 @@ function visitComponentMetadata(
       }
 
       const isInlineStyles = name === 'styles';
-      const styles = ts.visitNodes(node.initializer.elements, (node: ts.Expression) => {
+      const styles = ts.visitNodes(node.initializer.elements, (node) => {
         if (!ts.isStringLiteral(node) && !ts.isNoSubstitutionTemplateLiteral(node)) {
           return node;
         }
@@ -161,7 +165,7 @@ function visitComponentMetadata(
   }
 }
 
-export function getResourceUrl(node: ts.Expression, loader = ''): string | null {
+export function getResourceUrl(node: ts.Node, loader = ''): string | null {
   // only analyze strings
   if (!ts.isStringLiteral(node) && !ts.isNoSubstitutionTemplateLiteral(node)) {
     return null;

Original file line number	Diff line number	Diff line change
`@@ -27,7 +27,7 @@ export function findResources(sourceFile: ts.SourceFile): string[] {`
`27`	`27`
`28`	`28`	`ts.visitNodes(`
`29`	`29`	`(args[0] as ts.ObjectLiteralExpression).properties,`
`30`		`- (node: ts.ObjectLiteralElementLike) => {`
	`30`	`+ (node) => {`
`31`	`31`	`if (!ts.isPropertyAssignment(node) \|\| ts.isComputedPropertyName(node.name)) {`
`32`	`32`	`return node;`
`33`	`33`	`}`
`@@ -47,7 +47,7 @@ export function findResources(sourceFile: ts.SourceFile): string[] {`
`47`	`47`	`return node;`
`48`	`48`	`}`
`49`	`49`
`50`		`- ts.visitNodes(node.initializer.elements, (node: ts.Expression) => {`
	`50`	`+ ts.visitNodes(node.initializer.elements, (node) => {`
`51`	`51`	`const url = getResourceUrl(node);`
`52`	`52`
`53`	`53`	`if (url) {`