refactor(core): avoid wrapper around subscribe return value (#54387)

Reworks `ModelSignal.subscribe` so it doesn't have to wrap its value to look like a subscription.

PR Close #54387

goldens/public-api/core/index.md

@@ -1100,9 +1100,7 @@ export interface ModelSignal<T> extends WritableSignal<T> {
     // (undocumented)
     [SIGNAL]: ModelSignalNode<T>;
     // @deprecated (undocumented)
-    subscribe(callback: (value: T) => void): {
-        unsubscribe: () => void;
-    };
+    subscribe(callback: (value: T) => void): () => void;
 }
 
 // @public @deprecated

packages/core/src/authoring/model/model_signal.ts

@@ -43,7 +43,7 @@ export interface ModelSignal<T> extends WritableSignal<T> {
 
   // TODO(crisbeto): either make this a public API or mark as internal pending discussion.
   /** @deprecated Do not use, will be removed. */
-  subscribe(callback: (value: T) => void): {unsubscribe: () => void};
+  subscribe(callback: (value: T) => void): () => void;
 }
 
 /**
@@ -89,14 +89,11 @@ export function createModelSignal<T>(initialValue: T): ModelSignal<T> {
   getter.subscribe = (callback: (value: T) => void) => {
     subscriptions.push(callback);
 
-    // TODO(crisbeto): figure out if we can get rid of the object literal.
-    return {
-      unsubscribe: () => {
-        const index = subscriptions.indexOf(callback);
+    return () => {
+      const index = subscriptions.indexOf(callback);
 
-        if (index > -1) {
-          subscriptions.splice(index, 1);
-        }
+      if (index > -1) {
+        subscriptions.splice(index, 1);
       }
     };
   };

packages/core/src/render3/instructions/listener.ts

@@ -207,10 +207,16 @@ export function listenerInternal(
               directiveInstance.constructor.name}'.`);
         }
 
-        const subscription = (output as SubscribableOutput<unknown>).subscribe(listenerFn);
+        const subscriptionOrCallback =
+            (output as SubscribableOutput<unknown>).subscribe(listenerFn);
         const idx = lCleanup.length;
-        lCleanup.push(listenerFn, subscription);
-        tCleanup && tCleanup.push(eventName, tNode.index, idx, -(idx + 1));
+        lCleanup.push(listenerFn, subscriptionOrCallback);
+        if (tCleanup) {
+          // The cleanup function expects negative indexes to be
+          // of type Subscription while positive are cleanup functions.
+          const cleanupIdx = typeof subscriptionOrCallback === 'function' ? idx + 1 : -(idx + 1);
+          tCleanup.push(eventName, tNode.index, idx, cleanupIdx);
+        }
       }
     }
   }
@@ -277,8 +283,8 @@ function wrapListener(
 }
 
 /** Describes a subscribable output field value. */
-export interface SubscribableOutput<T> {
-  subscribe(listener: (v: T) => void): {unsubscribe: () => void;};
+interface SubscribableOutput<T> {
+  subscribe(listener: (v: T) => void): {unsubscribe: () => void;}|(() => void);
 }
 
 /**
@@ -287,7 +293,7 @@ export interface SubscribableOutput<T> {
  * For example, an `EventEmitter, a `Subject`, an `Observable` or an
  * `OutputEmitter`.
  */
-export function isOutputSubscribable(value: unknown): value is SubscribableOutput<unknown> {
+function isOutputSubscribable(value: unknown): value is SubscribableOutput<unknown> {
   return value != null &&
       typeof (value as Partial<SubscribableOutput<unknown>>).subscribe === 'function';
 }

packages/core/src/render3/node_manipulation.ts

@@ -442,7 +442,7 @@ function processCleanups(tView: TView, lView: LView): void {
         const targetIdx = tCleanup[i + 3];
         ngDevMode && assertNumber(targetIdx, 'cleanup target must be a number');
         if (targetIdx >= 0) {
-          // unregister
+          // Destroy anything whose teardown is a function call (e.g. QueryList, ModelSignal).
           lCleanup[targetIdx]();
         } else {
           // Subscription

packages/core/test/acceptance/authoring/model_inputs_spec.ts

@@ -564,4 +564,29 @@ describe('model inputs', () => {
       }),
     ]);
   });
+
+  it('should not throw for mixed model and output subscriptions', () => {
+    @Directive({selector: '[dir]', standalone: true})
+    class Dir {
+      model = model(0);
+      @Output() output = new EventEmitter();
+      model2 = model(0);
+      @Output() output2 = new EventEmitter();
+    }
+
+    @Component({
+      template: `
+        <div dir (model)="noop()" (output)="noop()" (model2)="noop()" (output2)="noop()"></div>
+      `,
+      standalone: true,
+      imports: [Dir],
+    })
+    class App {
+      noop() {}
+    }
+
+    const fixture = TestBed.createComponent(App);
+    fixture.detectChanges();
+    expect(() => fixture.destroy()).not.toThrow();
+  });
 });

packages/core/test/authoring/model_input_spec.ts

@@ -15,7 +15,7 @@ import {ModelSignal} from '@angular/core/src/authoring/model/model_signal';
  * in `ModelSignal`, but it's marked as internal which makes it unavailable in tests.
  */
 type SubscribableSignal<T> = ModelSignal<T>&{
-  subscribe(callback: (value: T) => void): {unsubscribe: () => void};
+  subscribe(callback: (value: T) => void): () => void;
 };
 
 describe('model signal', () => {
@@ -68,7 +68,7 @@ describe('model signal', () => {
     signal.set(1);
     expect(values).toEqual([1]);
 
-    subscription.unsubscribe();
+    subscription();
     signal.set(2);
     expect(values).toEqual([1]);
   });
@@ -103,11 +103,11 @@ describe('model signal', () => {
     signal2.set(1);
     expect(emitCount).toBe(2);
 
-    subscription1.unsubscribe();
+    subscription1();
     signal2.set(2);
     expect(emitCount).toBe(3);
 
-    subscription2.unsubscribe();
+    subscription2();
     signal2.set(3);
     expect(emitCount).toBe(3);
   });