fix(core): Fixed inject migration schematics for migrate destructured properties

Fixes angular#62626 - Properties used with the destructor are also managed during migration.

Author: aparzi

packages/core/schematics/ng-generate/inject-migration/migration.ts

@@ -280,6 +280,23 @@ function migrateClass(
   }
 }
 
+interface ParameterMigrationContext<T extends ts.Node = ts.ParameterDeclaration> {
+  node: T;
+  options: MigrationOptions;
+  localTypeChecker: ts.TypeChecker;
+  printer: ts.Printer;
+  tracker: ChangeTracker;
+  superCall: ts.CallExpression | null;
+  usedInSuper: boolean;
+  usedInConstructor: boolean;
+  usesOtherParams: boolean;
+  memberIndentation: string;
+  innerIndentation: string;
+  prependToConstructor: string[];
+  propsToAdd: string[];
+  afterSuper: string[];
+}
+
 /**
  * Migrates a single parameter to `inject()` DI.
  * @param node Parameter to be migrated.
@@ -312,9 +329,38 @@ function migrateParameter(
   propsToAdd: string[],
   afterSuper: string[],
 ): void {
-  if (!ts.isIdentifier(node.name)) {
+  const context: ParameterMigrationContext = {
+    node,
+    options,
+    localTypeChecker,
+    printer,
+    tracker,
+    superCall,
+    usedInSuper,
+    usedInConstructor,
+    usesOtherParams,
+    memberIndentation,
+    innerIndentation,
+    prependToConstructor,
+    propsToAdd,
+    afterSuper,
+  };
+
+  if (ts.isIdentifier(node.name)) {
+    migrateIdentifierParameter({
+      ...context,
+      node: node as ts.ParameterDeclaration & { name: ts.Identifier }
+    });
+  } else if (ts.isObjectBindingPattern(node.name)) {
+    migrateObjectBindingParameter(context);
+  } else {
     return;
   }
+}
+
+function migrateIdentifierParameter(context: ParameterMigrationContext<ts.ParameterDeclaration & { name: ts.Identifier }>): void {
+  const {node, options, localTypeChecker, printer, tracker, usedInConstructor, usesOtherParams} =
+    context;
 
   const name = node.name.text;
   const replacementCall = createInjectReplacementCall(
@@ -328,69 +374,211 @@ function migrateParameter(
 
   // If the parameter declares a property, we need to declare it (e.g. `private foo: Foo`).
   if (declaresProp) {
-    // We can't initialize the property if it's referenced within a `super` call or  it references
-    // other parameters. See the logic further below for the initialization.
-    const canInitialize = !usedInSuper && !usesOtherParams;
-    const prop = ts.factory.createPropertyDeclaration(
-      cloneModifiers(
-        node.modifiers?.filter((modifier) => {
-          // Strip out the DI decorators, as well as `public` which is redundant.
-          return !ts.isDecorator(modifier) && modifier.kind !== ts.SyntaxKind.PublicKeyword;
-        }),
-      ),
-      name,
-      // Don't add the question token to private properties since it won't affect interface implementation.
-      node.modifiers?.some((modifier) => modifier.kind === ts.SyntaxKind.PrivateKeyword)
-        ? undefined
-        : node.questionToken,
-      canInitialize ? undefined : node.type,
-      canInitialize ? ts.factory.createIdentifier(PLACEHOLDER) : undefined,
-    );
-
-    propsToAdd.push(
-      memberIndentation +
-        replaceNodePlaceholder(node.getSourceFile(), prop, replacementCall, printer),
-    );
+    handlePropertyDeclaration(context, name, replacementCall);
   }
 
   // If the parameter is referenced within the constructor, we need to declare it as a variable.
   if (usedInConstructor) {
-    if (usedInSuper) {
-      // Usages of `this` aren't allowed before `super` calls so we need to
-      // create a variable which calls `inject()` directly instead...
-      prependToConstructor.push(`${innerIndentation}const ${name} = ${replacementCall};`);
-
-      // ...then we can initialize the property after the `super` call.
-      if (declaresProp) {
-        afterSuper.push(`${innerIndentation}this.${name} = ${name};`);
-      }
-    } else if (declaresProp) {
-      // If the parameter declares a property (`private foo: foo`) and is used inside the class
-      // at the same time, we need to ensure that it's initialized to the value from the variable
-      // and that we only reference `this` after the `super` call.
-      const initializer = `${innerIndentation}const ${name} = this.${name};`;
-
-      if (superCall === null) {
-        prependToConstructor.push(initializer);
-      } else {
-        afterSuper.push(initializer);
-      }
-    } else {
-      // If the parameter is only referenced in the constructor, we
-      // don't need to declare any new properties.
-      prependToConstructor.push(`${innerIndentation}const ${name} = ${replacementCall};`);
-    }
+    handleConstructorUsage(context, name, replacementCall, declaresProp);
   } else if (usesOtherParams && declaresProp) {
-    const toAdd = `${innerIndentation}this.${name} = ${replacementCall};`;
+    handleParameterWithDependencies(context, name, replacementCall);
+  }
+}
+
+function handlePropertyDeclaration(
+  context: ParameterMigrationContext,
+  name: string,
+  replacementCall: string,
+): void {
+  const {node, memberIndentation, propsToAdd} = context;
+
+  const canInitialize = !context.usedInSuper && !context.usesOtherParams;
+  const prop = ts.factory.createPropertyDeclaration(
+    cloneModifiers(
+      node.modifiers?.filter((modifier) => {
+        return !ts.isDecorator(modifier) && modifier.kind !== ts.SyntaxKind.PublicKeyword;
+      }),
+    ),
+    name,
+    node.modifiers?.some((modifier) => modifier.kind === ts.SyntaxKind.PrivateKeyword)
+      ? undefined
+      : node.questionToken,
+    canInitialize ? undefined : node.type,
+    canInitialize ? ts.factory.createIdentifier(PLACEHOLDER) : undefined,
+  );
+
+  propsToAdd.push(
+    memberIndentation +
+      replaceNodePlaceholder(node.getSourceFile(), prop, replacementCall, context.printer),
+  );
+}
+
+function handleConstructorUsage(
+  context: ParameterMigrationContext,
+  name: string,
+  replacementCall: string,
+  declaresProp: boolean,
+): void {
+  const {innerIndentation, prependToConstructor, afterSuper, superCall} = context;
+
+  if (context.usedInSuper) {
+    // Usages of `this` aren't allowed before `super` calls so we need to
+    // create a variable which calls `inject()` directly instead...
+    prependToConstructor.push(`${innerIndentation}const ${name} = ${replacementCall};`);
+
+    if (declaresProp) {
+      afterSuper.push(`${innerIndentation}this.${name} = ${name};`);
+    }
+  } else if (declaresProp) {
+    // If the parameter declares a property (`private foo: foo`) and is used inside the class
+    // at the same time, we need to ensure that it's initialized to the value from the variable
+    // and that we only reference `this` after the `super` call.
+    const initializer = `${innerIndentation}const ${name} = this.${name};`;
 
     if (superCall === null) {
-      prependToConstructor.push(toAdd);
+      prependToConstructor.push(initializer);
     } else {
-      afterSuper.push(toAdd);
+      afterSuper.push(initializer);
     }
+  } else {
+    // If the parameter is only referenced in the constructor, we
+    // don't need to declare any new properties.
+    prependToConstructor.push(`${innerIndentation}const ${name} = ${replacementCall};`);
+  }
+}
+
+function handleParameterWithDependencies(
+  context: ParameterMigrationContext,
+  name: string,
+  replacementCall: string,
+): void {
+  const {innerIndentation, prependToConstructor, afterSuper, superCall} = context;
+
+  const toAdd = `${innerIndentation}this.${name} = ${replacementCall};`;
+
+  if (superCall === null) {
+    prependToConstructor.push(toAdd);
+  } else {
+    afterSuper.push(toAdd);
   }
 }
 
+function migrateObjectBindingParameter(context: ParameterMigrationContext): void {
+  const {node, options, localTypeChecker, printer, tracker} = context;
+
+  const replacementCall = createInjectReplacementCall(
+    node,
+    options,
+    localTypeChecker,
+    printer,
+    tracker,
+  );
+
+  for (const element of (node.name as ts.ObjectBindingPattern).elements) {
+    if (ts.isBindingElement(element) && ts.isIdentifier(element.name)) {
+      migrateBindingElement(context, element, replacementCall);
+    }
+  }
+}
+
+function migrateBindingElement(
+  context: ParameterMigrationContext,
+  element: ts.BindingElement,
+  replacementCall: string,
+): void {
+  const propertyName = (element.name as ts.Identifier).text;
+
+  // Determines how to access the property
+  const propertyAccess = element.propertyName
+    ? `${replacementCall}.${element.propertyName.getText()}`
+    : `${replacementCall}.${propertyName}`;
+
+  createPropertyForBindingElement(context, propertyName, propertyAccess);
+
+  if (context.usedInConstructor) {
+    handleConstructorUsageBindingElement(context, element, propertyName);
+  }
+}
+
+function handleConstructorUsageBindingElement(
+  context: ParameterMigrationContext,
+  element: ts.BindingElement,
+  propertyName: string,
+): void {
+  const {tracker, localTypeChecker, node: paramNode} = context;
+  const constructorDecl = paramNode.parent;
+
+  // Check in constructor or exist body content
+  if (!ts.isConstructorDeclaration(constructorDecl) || !constructorDecl.body) {
+    return;
+  }
+
+  // Get the unique "symbol" for our unstructured property.
+  const symbol = (localTypeChecker as ts.TypeChecker).getSymbolAtLocation(element.name);
+  if (!symbol) {
+    return;
+  }
+
+  // Visit recursive function navigate constructor
+  const visit = (node: ts.Node) => {
+    // Check if current node is identifier (variable)
+    if (ts.isIdentifier(node)) {
+      // Using the type checker, verify that this identifier refers
+      // exactly to our destructured parameter and is not the node of the original declaration.
+      if (localTypeChecker.getSymbolAtLocation(node) === symbol && node !== element.name) {
+        // If the identifier is used as a shorthand property in an object literal (e.g., { myVar }),
+        // must replace the entire `ShorthandPropertyAssignment` node
+        // with a `PropertyAssignment` (e.g., myVar: this.myVar).
+        if (ts.isShorthandPropertyAssignment(node.parent)) {
+          tracker.replaceNode(
+            node.parent,
+            ts.factory.createPropertyAssignment(
+              node,
+              ts.factory.createPropertyAccessExpression(ts.factory.createThis(), propertyName),
+            ),
+          );
+        } else {
+          // Otherwise, replace the identifier with `this.propertyName`.
+          tracker.replaceNode(
+            node,
+            ts.factory.createPropertyAccessExpression(ts.factory.createThis(), propertyName),
+          );
+        }
+      }
+    }
+    ts.forEachChild(node, visit);
+  };
+
+  visit(constructorDecl.body);
+}
+
+function createPropertyForBindingElement(
+  context: ParameterMigrationContext,
+  propertyName: string,
+  propertyAccess: string,
+): void {
+  const {node, memberIndentation, propsToAdd} = context;
+
+  const prop = ts.factory.createPropertyDeclaration(
+    cloneModifiers(
+      node.modifiers?.filter((modifier) => {
+        return !ts.isDecorator(modifier) && modifier.kind !== ts.SyntaxKind.PublicKeyword;
+      }),
+    ),
+    propertyName,
+    node.modifiers?.some((modifier) => modifier.kind === ts.SyntaxKind.PrivateKeyword)
+      ? undefined
+      : node.questionToken,
+    undefined,
+    ts.factory.createIdentifier(PLACEHOLDER),
+  );
+
+  propsToAdd.push(
+    memberIndentation +
+      replaceNodePlaceholder(node.getSourceFile(), prop, propertyAccess, context.printer),
+  );
+}
+
 /**
  * Creates a replacement `inject` call from a function parameter.
  * @param param Parameter for which to generate the `inject` call.

packages/core/schematics/test/inject_migration_spec.ts

@@ -479,6 +479,37 @@ describe('inject migration', () => {
     ]);
   });
 
+  it('should migrate destructoring property', async () => {
+    writeFile(
+      '/dir.ts',
+      [
+        `import { Directive, ElementRef } from '@angular/core';`,
+        ``,
+        `@Directive()`,
+        `class MyDir {`,
+        `  constructor({nativeElement}: ElementRef) {`,
+        `    console.log(nativeElement);`,
+        `  }`,
+        `}`,
+      ].join('\n'),
+    );
+
+    await runMigration();
+
+    expect(tree.readContent('/dir.ts').split('\n')).toEqual([
+      `import { Directive, ElementRef, inject } from '@angular/core';`,
+      ``,
+      `@Directive()`,
+      `class MyDir {`,
+      `  nativeElement = inject(ElementRef).nativeElement;`,
+      ``,
+      `  constructor() {`,
+      `    console.log(this.nativeElement);`,
+      `  }`,
+      `}`,
+    ]);
+  });
+
   it('should preserve the constructor if it has other expressions', async () => {
     writeFile(
       '/dir.ts',