fix(core): call DestroyRef on destroy callback if view is destroyed [patch] (#61061)

arturovt · mmalerba · commit dbb87026ca10 · 2025-04-30T08:52:25.000-07:00
Patch version of 5f7f046 PR Close #61061
diff --git a/packages/core/rxjs-interop/test/take_until_destroyed_spec.ts b/packages/core/rxjs-interop/test/take_until_destroyed_spec.ts
@@ -6,8 +6,17 @@
  * found in the LICENSE file at https://angular.dev/license
  */
 
-import {DestroyRef, EnvironmentInjector, Injector, runInInjectionContext} from '@angular/core';
-import {BehaviorSubject} from 'rxjs';
+import {
+  Component,
+  DestroyRef,
+  EnvironmentInjector,
+  inject,
+  Injector,
+  OnDestroy,
+  runInInjectionContext,
+} from '@angular/core';
+import {TestBed} from '@angular/core/testing';
+import {BehaviorSubject, finalize} from 'rxjs';
 
 import {takeUntilDestroyed} from '../src/take_until_destroyed';
 
@@ -76,4 +85,39 @@ describe('takeUntilDestroyed', () => {
 
     expect(unregisterFn).toHaveBeenCalled();
   });
+
+  // https://github.com/angular/angular/issues/54527
+  it('should unsubscribe after the current context has already been destroyed', async () => {
+    const recorder: any[] = [];
+
+    // Note that we need a "real" view for this test because, in other cases,
+    // `DestroyRef` would resolve to the root injector rather than to the
+    // `NodeInjectorDestroyRef`, where `lView` is used.
+    @Component({template: ''})
+    class TestComponent implements OnDestroy {
+      destroyRef = inject(DestroyRef);
+
+      source$ = new BehaviorSubject(0);
+
+      ngOnDestroy(): void {
+        Promise.resolve().then(() => {
+          this.source$
+            .pipe(
+              takeUntilDestroyed(this.destroyRef),
+              finalize(() => recorder.push('finalize()')),
+            )
+            .subscribe((value) => recorder.push(value));
+        });
+      }
+    }
+
+    const fixture = TestBed.createComponent(TestComponent);
+    fixture.destroy();
+
+    // Wait until the `ngOnDestroy` microtask is run.
+    await Promise.resolve();
+
+    // Ensure the `value` is not recorded, but unsubscribed immediately.
+    expect(recorder).toEqual(['finalize()']);
+  });
 });
diff --git a/packages/core/src/linker/destroy_ref.ts b/packages/core/src/linker/destroy_ref.ts
@@ -7,6 +7,7 @@
  */
 
 import {EnvironmentInjector} from '../di';
+import {isDestroyed} from '../render3/interfaces/type_checks';
 import {LView} from '../render3/interfaces/view';
 import {getLView} from '../render3/state';
 import {removeLViewOnDestroy, storeLViewOnDestroy} from '../render3/util/view_utils';
@@ -62,8 +63,28 @@ export class NodeInjectorDestroyRef extends DestroyRef {
   }
 
   override onDestroy(callback: () => void): () => void {
-    storeLViewOnDestroy(this._lView, callback);
-    return () => removeLViewOnDestroy(this._lView, callback);
+    const lView = this._lView;
+
+    // Checking if `lView` is already destroyed before storing the `callback` enhances
+    // safety and integrity for applications.
+    // If `lView` is destroyed, we call the `callback` immediately to ensure that
+    // any necessary cleanup is handled gracefully.
+    // With this approach, we're providing better reliability in managing resources.
+    // One of the use cases is `takeUntilDestroyed`, which aims to replace `takeUntil`
+    // in existing applications. While `takeUntil` can be safely called once the view
+    // is destroyed — resulting in no errors and finalizing the subscription depending
+    // on whether a subject or replay subject is used, replacing it with
+    // `takeUntilDestroyed` introduces a breaking change, as it throws an error if
+    // the `lView` is destroyed (https://github.com/angular/angular/issues/54527).
+    if (isDestroyed(lView)) {
+      callback();
+      // We return a "noop" callback, which, when executed, does nothing because
+      // we haven't stored anything on the `lView`, and thus there's nothing to remove.
+      return () => {};
+    }
+
+    storeLViewOnDestroy(lView, callback);
+    return () => removeLViewOnDestroy(lView, callback);
   }
 }
 
diff --git a/packages/core/test/acceptance/destroy_ref_spec.ts b/packages/core/test/acceptance/destroy_ref_spec.ts
@@ -248,25 +248,6 @@ describe('DestroyRef', () => {
     expect(onDestroyCalls).toBe(2);
   });
 
-  it('should throw when trying to register destroy callback on destroyed LView', () => {
-    @Component({
-      selector: 'test',
-      standalone: true,
-      template: ``,
-    })
-    class TestCmp {
-      constructor(public destroyRef: DestroyRef) {}
-    }
-
-    const fixture = TestBed.createComponent(TestCmp);
-    const destroyRef = fixture.componentRef.instance.destroyRef;
-    fixture.componentRef.destroy();
-
-    expect(() => {
-      destroyRef.onDestroy(() => {});
-    }).toThrowError('NG0911: View has already been destroyed.');
-  });
-
   it('should allow unregistration while destroying', () => {
     const destroyedLog: string[] = [];
 
