revert: fix(core): allow toSignal in reactive contexts (angular#52049)

devversion · atscott · commit ced66d400778 · 2023-10-05T11:08:05.000-07:00
Revert (with improvements of): dcf18dc We recently landed a change that allows `toSignal` to be called from within reactive contexts (e.g. `effect`/`computed`). After more thorough investigatio and consideration with the team, we feel like allowing `toSignal` to be called in such contexts is encouraging non-ideal / hard-to-notice code patterns. e.g. a new subscription to an observable is made every time `toSignal` is invoked. There is no caching done here. Additionally, multiple new subscriptions can trigger unintended side-effects- that may slow down the app, result in incorrect/unexpected behavior or perform unnecessary work. Users should instead move the `toSignal` call outside of the `computed` or `effect` and then read the signal values from within their `computed`. e.g. ```ts computed(() => { const smth = toSignal(coldObservable$) return smth() + 2; } ``` --> should instead be: ```ts const smth = toSignal(coldObsverable$); computed(() => smth() + 2); ``` In cases where a new subscription for each invocation is actually intended, a manual subscription can be made. That way it's also much more obvious to users that they are triggering side-effects every time, or causing new subscriptions. PR Close angular#52049
diff --git a/packages/core/rxjs-interop/src/to_signal.ts b/packages/core/rxjs-interop/src/to_signal.ts
@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {assertInInjectionContext, computed, DestroyRef, inject, Injector, signal, Signal, untracked, WritableSignal, ɵRuntimeError, ɵRuntimeErrorCode} from '@angular/core';
+import {assertInInjectionContext, assertNotInReactiveContext, computed, DestroyRef, inject, Injector, signal, Signal, WritableSignal, ɵRuntimeError, ɵRuntimeErrorCode} from '@angular/core';
 import {Observable, Subscribable} from 'rxjs';
 
 /**
@@ -159,6 +159,12 @@ export function toSignal<T>(
     options: ToSignalOptions<undefined>&{requireSync: true}): Signal<T>;
 export function toSignal<T, U = undefined>(
     source: Observable<T>|Subscribable<T>, options?: ToSignalOptions<U>): Signal<T|U> {
+  ngDevMode &&
+      assertNotInReactiveContext(
+          toSignal,
+          'Invoking `toSignal` causes new subscriptions every time. ' +
+              'Consider moving `toSignal` outside of the reactive context and read the signal value where needed.');
+
   const requiresCleanup = !options?.manualCleanup;
   requiresCleanup && !options?.injector && assertInInjectionContext(toSignal);
   const cleanupRef =
@@ -175,24 +181,28 @@ export function toSignal<T, U = undefined>(
     state = signal<State<T|U>>({kind: StateKind.Value, value: options?.initialValue as U});
   }
 
-  untracked(() => {
-    const sub = source.subscribe({
-      next: value => state.set({kind: StateKind.Value, value}),
-      error: error => state.set({kind: StateKind.Error, error}),
-      // Completion of the Observable is meaningless to the signal. Signals don't have a concept of
-      // "complete".
-    });
-
-    if (ngDevMode && options?.requireSync && state().kind === StateKind.NoValue) {
-      throw new ɵRuntimeError(
-          ɵRuntimeErrorCode.REQUIRE_SYNC_WITHOUT_SYNC_EMIT,
-          '`toSignal()` called with `requireSync` but `Observable` did not emit synchronously.');
-    }
-
-    // Unsubscribe when the current context is destroyed, if requested.
-    cleanupRef?.onDestroy(sub.unsubscribe.bind(sub));
+  // Note: This code cannot run inside a reactive context (see assertion above). If we'd support
+  // this, we would subscribe to the observable outside of the current reactive context, avoiding
+  // that side-effect signal reads/writes are attribute to the current consumer. The current
+  // consumer only needs to be notified when the `state` signal changes through the observable
+  // subscription. Additional context (related to async pipe):
+  // https://github.com/angular/angular/pull/50522.
+  const sub = source.subscribe({
+    next: value => state.set({kind: StateKind.Value, value}),
+    error: error => state.set({kind: StateKind.Error, error}),
+    // Completion of the Observable is meaningless to the signal. Signals don't have a concept of
+    // "complete".
   });
 
+  if (ngDevMode && options?.requireSync && state().kind === StateKind.NoValue) {
+    throw new ɵRuntimeError(
+        ɵRuntimeErrorCode.REQUIRE_SYNC_WITHOUT_SYNC_EMIT,
+        '`toSignal()` called with `requireSync` but `Observable` did not emit synchronously.');
+  }
+
+  // Unsubscribe when the current context is destroyed, if requested.
+  cleanupRef?.onDestroy(sub.unsubscribe.bind(sub));
+
   // The actual returned signal is a `computed` of the `State` signal, which maps the various states
   // to either values or errors.
   return computed(() => {
diff --git a/packages/core/rxjs-interop/test/to_signal_spec.ts b/packages/core/rxjs-interop/test/to_signal_spec.ts
@@ -6,8 +6,9 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {computed, EnvironmentInjector, Injector, runInInjectionContext} from '@angular/core';
+import {ChangeDetectionStrategy, Component, computed, EnvironmentInjector, Injector, runInInjectionContext} from '@angular/core';
 import {toSignal} from '@angular/core/rxjs-interop';
+import {TestBed} from '@angular/core/testing';
 import {BehaviorSubject, ReplaySubject, Subject} from 'rxjs';
 
 describe('toSignal()', () => {
@@ -109,17 +110,16 @@ describe('toSignal()', () => {
     expect(counter()).toBe(1);
   });
 
-  it('should allow toSignal creation in a reactive context - issue 51027', () => {
+  it('should not allow toSignal creation in a reactive context', () => {
     const counter$ = new BehaviorSubject(1);
-
-    const injector = Injector.create([]);
-
     const doubleCounter = computed(() => {
-      const counter = toSignal(counter$, {requireSync: true, injector});
+      const counter = toSignal(counter$, {requireSync: true});
       return counter() * 2;
     });
 
-    expect(doubleCounter()).toBe(2);
+    expect(() => doubleCounter())
+        .toThrowError(
+            /toSignal\(\) cannot be called from within a reactive context. Invoking `toSignal` causes new subscriptions every time./);
   });
 
   describe('with no initial value', () => {
@@ -173,6 +173,31 @@ describe('toSignal()', () => {
          expect(counter()).not.toBeNull();
        }));
   });
+
+  describe('in a @Component', () => {
+    it('should support `toSignal` as a class member initializer', () => {
+      @Component({
+        template: '{{counter()}}',
+        changeDetection: ChangeDetectionStrategy.OnPush,
+      })
+      class TestCmp {
+        // Component creation should not run inside the template effect/consumer,
+        // hence using `toSignal` should be allowed/supported.
+        counter$ = new Subject<number>();
+        counter = toSignal(this.counter$);
+      }
+
+      const fixture = TestBed.createComponent(TestCmp);
+      fixture.detectChanges();
+
+      expect(fixture.nativeElement.textContent).toBe('');
+
+      fixture.componentInstance.counter$.next(2);
+      fixture.detectChanges();
+
+      expect(fixture.nativeElement.textContent).toBe('2');
+    });
+  });
 });
 
 function test(fn: () => void|Promise<void>): () => Promise<void> {
