fix(migrations): do not migrate next calls in template if not an EventEmitter (#58631)

cexbrayat · pkozlowski-opensource · commit e6514b9f3dc1 · 2024-11-15T14:13:52.000+01:00
Fixes #58630 ```html <button (click)="someSubject.next()">Click</button> ``` was migrated to an `.emit` call, even if `someSubject` was not an `EventEmitter`. The issue was the same in host listeners. PR Close #58631
diff --git a/packages/core/schematics/migrations/output-migration/output-migration.spec.ts b/packages/core/schematics/migrations/output-migration/output-migration.spec.ts
@@ -257,6 +257,40 @@ describe('outputs', () => {
         });
       });
 
+      it('should _not_ migrate .next usages when not an EventEmitter in external template', async () => {
+        const tsContents = `
+              import {Component} from '@angular/core';
+              import {Subject} from 'rxjs';
+
+              @Component({
+                selector: 'test-cmp',
+                templateUrl: '/app.component.html'
+              })
+              export class TestCmpWithTemplate {
+                someChange = new Subject<void>();
+              }
+            `;
+        const htmlContents = `<button (click)="someChange.next()">click me</button>`;
+        const {fs} = await runTsurgeMigration(new OutputMigration(), [
+          {
+            name: absoluteFrom('/app.component.ts'),
+            isProgramRootFile: true,
+            contents: tsContents,
+          },
+          {
+            name: absoluteFrom('/app.component.html'),
+            contents: htmlContents,
+          },
+        ]);
+
+        const cmpActual = fs.readFile(absoluteFrom('/app.component.ts'));
+        const htmlActual = fs.readFile(absoluteFrom('/app.component.html'));
+
+        // nothing should have changed
+        expect(cmpActual).withContext(diffText(tsContents, cmpActual)).toEqual(tsContents);
+        expect(htmlActual).withContext(diffText(htmlContents, htmlActual)).toEqual(htmlContents);
+      });
+
       it('should migrate .next usage inside external template expressions', async () => {
         const {fs} = await runTsurgeMigration(new OutputMigration(), [
           {
@@ -333,6 +367,25 @@ describe('outputs', () => {
         });
       });
 
+      it('should _not_ migrate .next usage inside host listeners if not an EventEmitter', async () => {
+        await verifyNoChange(
+          `
+            import {Component, Output, EventEmitter} from '@angular/core';
+
+            @Component({
+              selector: 'test-cmp',
+              host: {
+                '(click)': 'someChange.next()'
+              },
+              template: ''
+            })
+            export class TestCmpWithTemplate {
+              someChange = new Subject();
+            }
+          `,
+        );
+      });
+
       it('should migrate .next usage in @HostListener', async () => {
         await verify({
           before: `
diff --git a/packages/core/schematics/migrations/output-migration/output-migration.ts b/packages/core/schematics/migrations/output-migration/output-migration.ts
@@ -112,7 +112,7 @@ export class OutputMigration extends TsurgeFunnelMigration<
     const knownFields: KnownFields<ClassFieldDescriptor> = {
       // Note: We don't support cross-target migration of `Partial<T>` usages.
       // This is an acceptable limitation for performance reasons.
-      shouldTrackClassReference: (node) => false,
+      shouldTrackClassReference: () => false,
       attemptRetrieveDescriptorFromSymbol: (s) => {
         const propDeclaration = getTargetPropertyDeclaration(s);
         if (propDeclaration !== null) {
@@ -278,7 +278,11 @@ export class OutputMigration extends TsurgeFunnelMigration<
       // detect .next usages that should be migrated to .emit in template and host binding expressions
       if (ref.kind === ReferenceKind.InTemplate) {
         const callExpr = checkNonTsReferenceCallsField(ref, 'next');
-        if (callExpr !== null) {
+        // TODO: here and below for host bindings, we should ideally filter in the global meta stage
+        // (instead of using the `outputFieldReplacements` map)
+        //  as technically, the call expression could refer to an output
+        //  from a whole different compilation unit (e.g. tsconfig.json).
+        if (callExpr !== null && outputFieldReplacements[ref.target.key] !== undefined) {
           addOutputReplacement(
             outputFieldReplacements,
             ref.target.key,
@@ -288,7 +292,7 @@ export class OutputMigration extends TsurgeFunnelMigration<
         }
       } else if (ref.kind === ReferenceKind.InHostBinding) {
         const callExpr = checkNonTsReferenceCallsField(ref, 'next');
-        if (callExpr !== null) {
+        if (callExpr !== null && outputFieldReplacements[ref.target.key] !== undefined) {
           addOutputReplacement(
             outputFieldReplacements,
             ref.target.key,
