angular · crisbeto · Oct 28, 2024 · Oct 28, 2024 · Oct 28, 2024 · Oct 28, 2024
diff --git a/packages/core/schematics/ng-generate/inject-migration/analysis.ts b/packages/core/schematics/ng-generate/inject-migration/analysis.ts
@@ -128,7 +128,7 @@ export function analyzeFile(sourceFile: ts.SourceFile, localTypeChecker: ts.Type
 export function getConstructorUnusedParameters(
   declaration: ts.ConstructorDeclaration,
   localTypeChecker: ts.TypeChecker,
-  removedStatements: Set<ts.Statement> | null,
+  removedStatements: Set<ts.Statement>,
 ): Set<ts.Declaration> {
   const accessedTopLevelParameters = new Set<ts.Declaration>();
   const topLevelParameters = new Set<ts.Declaration>();
@@ -149,7 +149,7 @@ export function getConstructorUnusedParameters(
 
   declaration.body.forEachChild(function walk(node) {
     // Don't descend into statements that were removed already.
-    if (removedStatements && ts.isStatement(node) && removedStatements.has(node)) {
+    if (ts.isStatement(node) && removedStatements.has(node)) {
       return;
     }
 

diff --git a/packages/core/schematics/ng-generate/inject-migration/internal.ts b/packages/core/schematics/ng-generate/inject-migration/internal.ts
@@ -32,8 +32,12 @@ export function findUninitializedPropertiesToCombine(
   node: ts.ClassDeclaration,
   constructor: ts.ConstructorDeclaration,
   localTypeChecker: ts.TypeChecker,
-): Map<ts.PropertyDeclaration, ts.Expression> | null {
-  let result: Map<ts.PropertyDeclaration, ts.Expression> | null = null;
+): {
+  toCombine: Map<ts.PropertyDeclaration, ts.Expression>;
+  toHoist: ts.PropertyDeclaration[];
+} | null {
+  let toCombine: Map<ts.PropertyDeclaration, ts.Expression> | null = null;
+  let toHoist: ts.PropertyDeclaration[] = [];
 
   const membersToDeclarations = new Map<string, ts.PropertyDeclaration>();
   for (const member of node.members) {
@@ -47,25 +51,43 @@ export function findUninitializedPropertiesToCombine(
   }
 
   if (membersToDeclarations.size === 0) {
-    return result;
+    return null;
   }
 
   const memberInitializers = getMemberInitializers(constructor);
   if (memberInitializers === null) {
-    return result;
+    return null;
   }
 
-  for (const [name, initializer] of memberInitializers.entries()) {
-    if (
-      membersToDeclarations.has(name) &&
-      !hasLocalReferences(initializer, constructor, localTypeChecker)
-    ) {
-      result = result || new Map();
-      result.set(membersToDeclarations.get(name)!, initializer);
+  for (const [name, decl] of membersToDeclarations.entries()) {
+    if (memberInitializers.has(name)) {
+      const initializer = memberInitializers.get(name)!;
+
+      if (!hasLocalReferences(initializer, constructor, localTypeChecker)) {
+        toCombine = toCombine || new Map();
+        toCombine.set(membersToDeclarations.get(name)!, initializer);
+      }
+    } else {
+      // Mark members that have no initializers and can't be combined to be hoisted above the
+      // injected members. This is either a no-op or it allows us to avoid some patterns internally
+      // like the following:
+      // ```
+      // class Foo {
+      //   publicFoo: Foo;
+      //   private privateFoo: Foo;
+      //
+      //   constructor() {
+      //     this.initializePrivateFooSomehow();
+      //     this.publicFoo = this.privateFoo;
+      //   }
+      // }
+      // ```
+      toHoist.push(decl);
     }
   }
 
-  return result;
+  // If no members need to be combined, none need to be hoisted either.
+  return toCombine === null ? null : {toCombine, toHoist};
 }
 
 /**
@@ -136,7 +158,7 @@ function hasLocalReferences(
   const sourceFile = root.getSourceFile();
   let hasLocalRefs = false;
 
-  root.forEachChild(function walk(node) {
+  const walk = (node: ts.Node) => {
     // Stop searching if we know that it has local references.
     if (hasLocalRefs) {
       return;
@@ -171,7 +193,9 @@ function hasLocalReferences(
     if (!hasLocalRefs) {
       node.forEachChild(walk);
     }
-  });
+  };
+
+  walk(root);
 
   return hasLocalRefs;
 }

diff --git a/packages/core/schematics/ng-generate/inject-migration/migration.ts b/packages/core/schematics/ng-generate/inject-migration/migration.ts
@@ -82,34 +82,20 @@ export function migrateFile(sourceFile: ts.SourceFile, options: MigrationOptions
   const tracker = new ChangeTracker(printer);
 
   analysis.classes.forEach(({node, constructor, superCall}) => {
-    let removedStatements: Set<ts.Statement> | null = null;
+    const memberIndentation = getLeadingLineWhitespaceOfNode(node.members[0]);
+    const prependToClass: string[] = [];
+    const removedStatements = new Set<ts.Statement>();
 
     if (options._internalCombineMemberInitializers) {
-      findUninitializedPropertiesToCombine(node, constructor, localTypeChecker)?.forEach(
-        (initializer, property) => {
-          const statement = closestNode(initializer, ts.isStatement);
-
-          if (!statement) {
-            return;
-          }
-
-          const newProperty = ts.factory.createPropertyDeclaration(
-            cloneModifiers(property.modifiers),
-            cloneName(property.name),
-            property.questionToken,
-            property.type,
-            initializer,
-          );
-          tracker.replaceText(
-            statement.getSourceFile(),
-            statement.getFullStart(),
-            statement.getFullWidth(),
-            '',
-          );
-          tracker.replaceNode(property, newProperty);
-          removedStatements = removedStatements || new Set();
-          removedStatements.add(statement);
-        },
+      applyInternalOnlyChanges(
+        node,
+        constructor,
+        localTypeChecker,
+        tracker,
+        printer,
+        removedStatements,
+        prependToClass,
+        memberIndentation,
       );
     }
 
@@ -118,6 +104,8 @@ export function migrateFile(sourceFile: ts.SourceFile, options: MigrationOptions
       constructor,
       superCall,
       options,
+      memberIndentation,
+      prependToClass,
       removedStatements,
       localTypeChecker,
       printer,
@@ -141,6 +129,8 @@ export function migrateFile(sourceFile: ts.SourceFile, options: MigrationOptions
  * @param constructor Reference to the class' constructor node.
  * @param superCall Reference to the constructor's `super()` call, if any.
  * @param options Options used to configure the migration.
+ * @param memberIndentation Indentation string of the members of the class.
+ * @param prependToClass Text that should be prepended to the class.
  * @param removedStatements Statements that have been removed from the constructor already.
  * @param localTypeChecker Type checker set up for the specific file.
  * @param printer Printer used to output AST nodes as strings.
@@ -151,7 +141,9 @@ function migrateClass(
   constructor: ts.ConstructorDeclaration,
   superCall: ts.CallExpression | null,
   options: MigrationOptions,
-  removedStatements: Set<ts.Statement> | null,
+  memberIndentation: string,
+  prependToClass: string[],
+  removedStatements: Set<ts.Statement>,
   localTypeChecker: ts.TypeChecker,
   printer: ts.Printer,
   tracker: ChangeTracker,
@@ -173,14 +165,12 @@ function migrateClass(
   const superParameters = superCall
     ? getSuperParameters(constructor, superCall, localTypeChecker)
     : null;
-  const memberIndentation = getLeadingLineWhitespaceOfNode(node.members[0]);
-  const removedStatementCount = removedStatements?.size || 0;
-  const innerReference =
-    superCall ||
-    constructor.body?.statements.find((statement) => !removedStatements?.has(statement)) ||
-    constructor;
+  const removedStatementCount = removedStatements.size;
+  const firstConstructorStatement = constructor.body?.statements.find(
+    (statement) => !removedStatements.has(statement),
+  );
+  const innerReference = superCall || firstConstructorStatement || constructor;
   const innerIndentation = getLeadingLineWhitespaceOfNode(innerReference);
-  const propsToAdd: string[] = [];
   const prependToConstructor: string[] = [];
   const afterSuper: string[] = [];
   const removedMembers = new Set<ts.ClassElement>();
@@ -201,7 +191,7 @@ function migrateClass(
       memberIndentation,
       innerIndentation,
       prependToConstructor,
-      propsToAdd,
+      prependToClass,
       afterSuper,
     );
   }
@@ -227,14 +217,23 @@ function migrateClass(
     if (prependToConstructor.length > 0) {
       tracker.insertText(
         sourceFile,
-        innerReference.getFullStart(),
+        (firstConstructorStatement || innerReference).getFullStart(),
         `\n${prependToConstructor.join('\n')}\n`,
       );
     }
   }
 
   if (afterSuper.length > 0 && superCall !== null) {
-    tracker.insertText(sourceFile, superCall.getEnd() + 1, `\n${afterSuper.join('\n')}\n`);
+    // Note that if we can, we should insert before the next statement after the `super` call,
+    // rather than after the end of it. Otherwise the string buffering implementation may drop
+    // the text if the statement after the `super` call is being deleted. This appears to be because
+    // the full start of the next statement appears to always be the end of the `super` call plus 1.
+    const nextStatement = getNextPreservedStatement(superCall, removedStatements);
+    tracker.insertText(
+      sourceFile,
+      nextStatement ? nextStatement.getFullStart() : superCall.getEnd() + 1,
+      `\n${afterSuper.join('\n')}\n`,
+    );
   }
 
   // Need to resolve this once all constructor signatures have been removed.
@@ -249,21 +248,21 @@ function migrateClass(
 
     // The new signature always has to be right before the constructor implementation.
     if (memberReference === constructor) {
-      propsToAdd.push(extraSignature);
+      prependToClass.push(extraSignature);
     } else {
       tracker.insertText(sourceFile, constructor.getFullStart(), '\n' + extraSignature);
     }
   }
 
-  if (propsToAdd.length > 0) {
+  if (prependToClass.length > 0) {
     if (removedMembers.size === node.members.length) {
-      tracker.insertText(sourceFile, constructor.getEnd() + 1, `${propsToAdd.join('\n')}\n`);
+      tracker.insertText(sourceFile, constructor.getEnd() + 1, `${prependToClass.join('\n')}\n`);
     } else {
       // Insert the new properties after the first member that hasn't been deleted.
       tracker.insertText(
         sourceFile,
         memberReference.getFullStart(),
-        `\n${propsToAdd.join('\n')}\n`,
+        `\n${prependToClass.join('\n')}\n`,
       );
     }
   }
@@ -685,3 +684,93 @@ function canRemoveConstructor(
     (statementCount === 1 && superCall !== null && superCall.arguments.length === 0)
   );
 }
+
+/**
+ * Gets the next statement after a node that *won't* be deleted by the migration.
+ * @param startNode Node from which to start the search.
+ * @param removedStatements Statements that have been removed by the migration.
+ * @returns
+ */
+function getNextPreservedStatement(
+  startNode: ts.Node,
+  removedStatements: Set<ts.Statement>,
+): ts.Statement | null {
+  const body = closestNode(startNode, ts.isBlock);
+  const closestStatement = closestNode(startNode, ts.isStatement);
+  if (body === null || closestStatement === null) {
+    return null;
+  }
+
+  const index = body.statements.indexOf(closestStatement);
+  if (index === -1) {
+    return null;
+  }
+
+  for (let i = index + 1; i < body.statements.length; i++) {
+    if (!removedStatements.has(body.statements[i])) {
+      return body.statements[i];
+    }
+  }
+
+  return null;
+}
+
+/**
+ * Applies the internal-specific migrations to a class.
+ * @param node Class being migrated.
+ * @param constructor The migrated class' constructor.
+ * @param localTypeChecker File-specific type checker.
+ * @param tracker Object keeping track of the changes.
+ * @param printer Printer used to output AST nodes as text.
+ * @param removedStatements Statements that have been removed by the migration.
+ * @param prependToClass Text that will be prepended to a class.
+ * @param memberIndentation Indentation string of the class' members.
+ */
+function applyInternalOnlyChanges(
+  node: ts.ClassDeclaration,
+  constructor: ts.ConstructorDeclaration,
+  localTypeChecker: ts.TypeChecker,
+  tracker: ChangeTracker,
+  printer: ts.Printer,
+  removedStatements: Set<ts.Statement>,
+  prependToClass: string[],
+  memberIndentation: string,
+) {
+  const result = findUninitializedPropertiesToCombine(node, constructor, localTypeChecker);
+
+  result?.toCombine.forEach((initializer, property) => {
+    const statement = closestNode(initializer, ts.isStatement);
+
+    if (!statement) {
+      return;
+    }
+
+    const newProperty = ts.factory.createPropertyDeclaration(
+      cloneModifiers(property.modifiers),
+      cloneName(property.name),
+      property.questionToken,
+      property.type,
+      initializer,
+    );
+    tracker.replaceText(
+      statement.getSourceFile(),
+      statement.getFullStart(),
+      statement.getFullWidth(),
+      '',
+    );
+    tracker.replaceNode(property, newProperty);
+    removedStatements.add(statement);
+  });
+
+  result?.toHoist.forEach((decl) => {
+    prependToClass.push(
+      memberIndentation + printer.printNode(ts.EmitHint.Unspecified, decl, decl.getSourceFile()),
+    );
+    tracker.replaceText(decl.getSourceFile(), decl.getFullStart(), decl.getFullWidth(), '');
+  });
+
+  // If we added any hoisted properties, separate them visually with a new line.
+  if (prependToClass.length > 0) {
+    prependToClass.push('');
+  }
+}