8000 fix(core): call DestroyRef on destroy callback if view is destroyed [patch] by arturovt · Pull Request #61061 · angular/angular · GitHub
[go: up one dir, main page]

Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 46 additions & 2 deletions packages/core/rxjs-interop/test/take_until_destroyed_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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()']);
});
});
25 changes: 23 additions & 2 deletions packages/core/src/linker/destroy_ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
}
}

Expand Down
19 changes: 0 additions & 19 deletions packages/core/test/acceptance/destroy_ref_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = [];

Expand Down
0