refactor(core): address PR feedback (angular#54252)

crisbeto · thePunderWoman · commit 551c5791f850 · 2024-02-07T16:36:12.000Z
Addresses the feedback from angular#54252. PR Close angular#54252
diff --git a/packages/compiler-cli/src/ngtsc/annotations/directive/src/model_function.ts b/packages/compiler-cli/src/ngtsc/annotations/directive/src/model_function.ts
@@ -34,6 +34,7 @@ export function tryParseSignalModelMapping(
   const bindingPropertyName = options?.alias ?? classPropertyName;
 
   return {
+    call: model.call,
     input: {
       isSignal: true,
       transform: null,
diff --git a/packages/compiler-cli/src/ngtsc/annotations/directive/src/shared.ts b/packages/compiler-cli/src/ngtsc/annotations/directive/src/shared.ts
@@ -1127,7 +1127,8 @@ function parseOutputFields(
           `Using @Output with a model input is not allowed.`);
     }
 
-    const queryNode = decoratorOutput?.decorator.node ?? initializerOutput?.call;
+    const queryNode =
+        decoratorOutput?.decorator.node ?? initializerOutput?.call ?? modelMapping?.call;
     if (queryNode !== undefined && member.isStatic) {
       throw new FatalDiagnosticError(
           ErrorCode.INCORRECTLY_DECLARED_ON_STATIC_MEMBER, queryNode,
@@ -1148,7 +1149,8 @@ function parseOutputFields(
 
     // Validate that initializer-based outputs are not accidentally declared
     // in the `outputs` class metadata.
-    if (initializerOutput !== null && outputsFromMeta.hasOwnProperty(member.name)) {
+    if (((initializerOutput !== null || modelMapping !== null) &&
+         outputsFromMeta.hasOwnProperty(member.name))) {
       throw new FatalDiagnosticError(
           ErrorCode.INITIALIZER_API_DECORATOR_METADATA_COLLISION, member.node ?? clazz,
           `Output "${member.name}" is unexpectedly declared in @${classDecorator.name} as well.`);
diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts
@@ -152,14 +152,11 @@ export type InputMapping = InputOrOutput&{
 
 /** Metadata for a model mapping. */
 export interface ModelMapping {
-  /** Information about the input declared by the model. */
-  input: InputOrOutput&{
-    /** Whether the input declared by the model is required. */
-    required: boolean;
+  /** Node defining the model mapping. */
+  call: ts.CallExpression;
 
-    /** Model inputs can't have transforms. Keeping it here for consistency. */
-    transform: null;
-  };
+  /** Information about the input declared by the model. */
+  input: InputMapping;
 
   /** Information about the output implicitly declared by the model. */
   output: InputOrOutput;
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/output_function_diagnostics.spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/output_function_diagnostics.spec.ts
@@ -30,6 +30,24 @@ runInEachFileSystem(() => {
           `TestComponent.html(1, 24): Property 'x' does not exist on type 'void'.`,
         ],
       },
+      {
+        id: 'two way data binding, invalid',
+        inputs: {'value': {type: 'InputSignal<string>', isSignal: true}},
+        outputs: {'valueChange': {type: 'OutputEmitter<string>'}},
+        template: `<div dir [(value)]="bla">`,
+        component: `bla = true;`,
+        expected: [
+          `TestComponent.html(1, 12): Type 'boolean' is not assignable to type 'string | WritableSignal<string>'.`,
+        ],
+      },
+      {
+        id: 'two way data binding, valid',
+        inputs: {'value': {type: 'InputSignal<string>', isSignal: true}},
+        outputs: {'valueChange': {type: 'OutputEmitter<string>'}},
+        template: `<div dir [(value)]="bla">`,
+        component: `bla: string = ''`,
+        expected: [],
+      },
       {
         id: 'complex output object',
         outputs: {'evt': {type: 'OutputEmitter<{works: boolean}>'}},
diff --git a/packages/compiler-cli/test/initializer_api_transforms_spec.ts b/packages/compiler-cli/test/initializer_api_transforms_spec.ts
@@ -16,7 +16,7 @@ import {MockAotContext, MockCompilerHost} from './mocks';
 const TEST_FILE_INPUT = '/test.ts';
 const TEST_FILE_OUTPUT = `/test.js`;
 
-describe('signal inputs metadata transform', () => {
+describe('initializer API metadata transform', () => {
   let host: MockCompilerHost;
   let context: MockAotContext;
 
diff --git a/packages/compiler-cli/test/ngtsc/authoring_spec.ts b/packages/compiler-cli/test/ngtsc/authoring_spec.ts
@@ -342,7 +342,7 @@ runInEachFileSystem(() => {
           '{ "value": "valueChange"; }, never, never, false, never>;');
     });
 
-    it('should declare an input/output pair for a field initialized to an alised model()', () => {
+    it('should declare an input/output pair for a field initialized to an aliased model()', () => {
       env.write('test.ts', `
         import {Directive, model} from '@angular/core';
 
diff --git a/packages/core/src/authoring/model/model_signal.ts b/packages/core/src/authoring/model/model_signal.ts
@@ -42,7 +42,7 @@ export interface ModelSignal<T> extends WritableSignal<T> {
   [ɵINPUT_SIGNAL_BRAND_WRITE_TYPE]: T;
 
   /**
-   * Subscribes to changes in the model's value.
+   * Subscribes to changes in the model's value. Used by listener instructions at runtime.
    * @internal
    */
   subscribe(callback: (value: T) => void): {unsubscribe: () => void};
@@ -56,6 +56,7 @@ export interface ModelSignal<T> extends WritableSignal<T> {
  * @param options Additional options for the model.
  */
 export function createModelSignal<T>(initialValue: T): ModelSignal<T> {
+  const subscriptions: ((value: T) => void)[] = [];
   const node: ModelSignalNode<T> = Object.create(MODEL_SIGNAL_NODE);
 
   node.value = initialValue;
@@ -73,8 +74,6 @@ export function createModelSignal<T>(initialValue: T): ModelSignal<T> {
   }
 
   function notifySubscribers(value: T): void {
-    const subscriptions = node.subscriptions;
-
     for (let i = 0; i < subscriptions.length; i++) {
       subscriptions[i](value);
     }
@@ -95,15 +94,15 @@ export function createModelSignal<T>(initialValue: T): ModelSignal<T> {
   };
 
   getter.subscribe = (callback: (value: T) => void) => {
-    node.subscriptions.push(callback);
+    subscriptions.push(callback);
 
     // TODO(crisbeto): figure out if we can get rid of the object literal.
     return {
       unsubscribe: () => {
-        const index = node.subscriptions.indexOf(callback);
+        const index = subscriptions.indexOf(callback);
 
         if (index > -1) {
-          node.subscriptions.splice(index, 1);
+          subscriptions.splice(index, 1);
         }
       }
     };
diff --git a/packages/core/src/authoring/model/model_signal_node.ts b/packages/core/src/authoring/model/model_signal_node.ts
@@ -15,9 +15,6 @@ export const REQUIRED_UNSET_VALUE = /* @__PURE__ */ Symbol('ModelSignalNode#UNSE
  * signals by adding the ability to track subscriptions and to be required.
  */
 export interface ModelSignalNode<T> extends SignalNode<T|typeof REQUIRED_UNSET_VALUE> {
-  /** Currently-active subscriptions on the model. */
-  subscriptions: ((value: T) => void)[];
-
   /** Used by the runtime to write a value to the signal input. */
   applyValueToInputSignal: (node: ModelSignalNode<T>, value: T) => void;
 }
@@ -28,7 +25,6 @@ export interface ModelSignalNode<T> extends SignalNode<T|typeof REQUIRED_UNSET_V
 export const MODEL_SIGNAL_NODE: ModelSignalNode<unknown> = /* @__PURE__ */ (() => {
   return {
     ...SIGNAL_NODE,
-    subscriptions: [],
 
     // TODO(crisbeto): figure out how to avoid this.
     // Maybe set an input flag that the value is writeable?
diff --git a/packages/core/test/acceptance/authoring/model_inputs_spec.ts b/packages/core/test/acceptance/authoring/model_inputs_spec.ts
@@ -36,7 +36,6 @@ describe('model inputs', () => {
 
     // Changing the value from within the directive.
     host.dir.value.set(2);
-    fixture.detectChanges();
     expect(host.value()).toBe(2);
     expect(host.dir.value()).toBe(2);
 
@@ -73,7 +72,6 @@ describe('model inputs', () => {
 
     // Changing the value from within the directive.
     host.dir.value.set(2);
-    fixture.detectChanges();
     expect(host.value).toBe(2);
     expect(host.dir.value()).toBe(2);
 
@@ -456,7 +454,7 @@ describe('model inputs', () => {
     expect(host.dir.value()).toBe(3);
   });
 
-  it('should allow a two-way-bound signal to be bound in the template', () => {
+  it('should reflect changes to a two-way-bound signal in the DOM', () => {
     @Directive({
       selector: '[dir]',
       standalone: true,
@@ -501,9 +499,9 @@ describe('model inputs', () => {
     class Dir implements OnChanges {
       value = model(0);
 
-      ngOnChanges(allCahnges: SimpleChanges): void {
-        if (allCahnges['value']) {
-          changes.push(allCahnges['value']);
+      ngOnChanges(allChanges: SimpleChanges): void {
+        if (allChanges['value']) {
+          changes.push(allChanges['value']);
         }
       }
     }
diff --git a/packages/core/test/authoring/model_input_spec.ts b/packages/core/test/authoring/model_input_spec.ts
@@ -89,6 +89,29 @@ describe('model signal', () => {
     expect(values).toEqual([1, 2]);
   });
 
+  it('should not share subscriptions between models', () => {
+    let emitCount = 0;
+    const signal1 = model(0) as SubscribableSignal<number>;
+    const signal2 = model(0) as SubscribableSignal<number>;
+    const callback = () => emitCount++;
+    const subscription1 = signal1.subscribe(callback);
+    const subscription2 = signal2.subscribe(callback);
+
+    signal1.set(1);
+    expect(emitCount).toBe(1);
+
+    signal2.set(1);
+    expect(emitCount).toBe(2);
+
+    subscription1.unsubscribe();
+    signal2.set(2);
+    expect(emitCount).toBe(3);
+
+    subscription2.unsubscribe();
+    signal2.set(3);
+    expect(emitCount).toBe(3);
+  });
+
   it('should throw if there is no value for required model', () => {
     const signal = model.required();
 
diff --git a/packages/core/test/authoring/signal_model_signature_test.ts b/packages/core/test/authoring/signal_model_signature_test.ts
@@ -51,12 +51,10 @@ export class InputSignatureTest {
 
   /** unknown */
   noInitialValueNoType = model();
+
   /** string */
   requiredNoInitialValueNoType = model.required<string>();
 
-  /** string | undefined */
-  assignableToWritableSignal: WritableSignal<string|undefined> = model<string>();
-
   /** @internal */
   __shouldErrorIfInitialValueWithRequired = model.required({
     // @ts-expect-error
diff --git a/packages/core/test/authoring/type_tester.ts b/packages/core/test/authoring/type_tester.ts
@@ -67,6 +67,7 @@ async function main() {
       'signal_input_signature_test',
       v => v.includes(',') ? `InputSignalWithTransform<${v}>` : `InputSignal<${v}>`, true);
   failing ||= testFile('signal_queries_signature_test', v => `Signal<${v}>`, false);
+  failing ||= testFile('signal_model_signature_test', v => `ModelSignal<${v}>`, false);
 
   if (failing) {
     throw new Error('Failing assertions');
