From aa0e54fe971b1cb7e0372674ff59eed47f835f41 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Sun, 11 Apr 2021 11:15:12 +0200 Subject: [PATCH] fix(core): error if DebugRenderer2.destroyNode is called twice in a row (#41565) Fixes an error that will be thrown if `DebugRenderer2.destroyNode` is called with a node that has already been destroyed. The error happened, because we had a non-null assertion, even though the value can be null. Note that this fix applies only to ViewEngine, because Ivy doesn't provide the `DebugRenderer2`. I decided to resolve it, because it fix is straightforward and this error has been showing up in our logs for a long time now, making actual errors harder to find. PR Close #41565 --- packages/core/src/view/services.ts | 12 ++++++++---- packages/core/test/debug/debug_node_spec.ts | 17 +++++++++++++++++ 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/packages/core/src/view/services.ts b/packages/core/src/view/services.ts index 1116f444e1..d62efa68ea 100644 --- a/packages/core/src/view/services.ts +++ b/packages/core/src/view/services.ts @@ -703,11 +703,15 @@ export class DebugRenderer2 implements Renderer2 { } destroyNode(node: any) { - const debugNode = getDebugNode(node)!; - removeDebugNodeFromIndex(debugNode); - if (debugNode instanceof DebugNode__PRE_R3__) { - debugNode.listeners.length = 0; + const debugNode = getDebugNode(node); + + if (debugNode) { + removeDebugNodeFromIndex(debugNode); + if (debugNode instanceof DebugNode__PRE_R3__) { + debugNode.listeners.length = 0; + } } + if (this.delegate.destroyNode) { this.delegate.destroyNode(node); } diff --git a/packages/core/test/debug/debug_node_spec.ts b/packages/core/test/debug/debug_node_spec.ts index af6ffc0717..ca44fa6187 100644 --- a/packages/core/test/debug/debug_node_spec.ts +++ b/packages/core/test/debug/debug_node_spec.ts @@ -208,6 +208,7 @@ class TestApp { width = 200; color = 'red'; isClosed = true; + constructor(public renderer: Renderer2) {} } @Component({selector: 'test-cmpt', template: ``}) @@ -618,6 +619,22 @@ class TestCmptWithPropInterpolation { expect(fixture.debugElement.query(By.css('.myclass'))).toBeTruthy(); }); + it('should not throw when calling DebugRenderer2.destroyNode twice in a row', () => { + const fixture = TestBed.createComponent(TestApp); + fixture.detectChanges(); + const firstChild = fixture.debugElement.children[0]; + const renderer = fixture.componentInstance.renderer; + + expect(firstChild).toBeTruthy(); + expect(() => { + // `destroyNode` needs to be null checked, because only ViewEngine provides a + // `DebugRenderer2` which has the behavior we're testing for. Ivy provides + // `BaseAnimationRenderer` which doesn't have the issue. + renderer.destroyNode?.(firstChild); + renderer.destroyNode?.(firstChild); + }).not.toThrow(); + }); + describe('DebugElement.query with dynamically created descendant elements', () => { let fixture: ComponentFixture<{}>; beforeEach(() => {