Skip to content

Commit 184844e

Browse files
authored
Merge pull request #7 from Microsoft/fix-unmounting-nodes
Fix React nodes being removed from DOM when they are still needed
2 parents 04b0e62 + 0a13bee commit 184844e

File tree

7 files changed

+108
-59
lines changed

7 files changed

+108
-59
lines changed

apps/demo/src/app/app.component.html

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -16,19 +16,31 @@ <h2>Getting up and running...</h2>
1616
</div>
1717

1818
<div>
19+
<fab-pivot>
20+
<fab-pivot-item headerText="Tab 1">
21+
<div>Tab 1's content</div>
22+
</fab-pivot-item>
23+
<fab-pivot-item headerText="Tab 2">
24+
<div>Tab 2's content</div>
25+
</fab-pivot-item>
26+
<fab-pivot-item headerText="Tab 3">
27+
<div>Tab 3's content</div>
28+
</fab-pivot-item>
29+
</fab-pivot>
30+
1931
<fab-command-bar>
2032
<items>
2133
<fab-command-bar-item key="run" text="Run" [iconProps]="{ iconName: 'CaretRight' }" [disabled]="runDisabled"></fab-command-bar-item>
2234
<fab-command-bar-item key="new" text="New" [iconProps]="{ iconName: 'Add' }" (click)="onNewClicked()"></fab-command-bar-item>
23-
<fab-command-bar-item key="save" text="Save" [iconProps]="{ iconName: 'Save' }" [subMenuProps]="saveSubMenuProps">
24-
<contextual-menu-item *ngIf="runDisabled" key="sometimesVisible" text="woosh"></contextual-menu-item>
25-
<contextual-menu-item key="save" text="Save"></contextual-menu-item>
26-
<contextual-menu-item key="save-as" text="Save As" (click)="onSaveAsClicked()">
27-
<contextual-menu-item key="save-as-1" text="Save As 1" (click)="onSaveAsFirstClicked()"></contextual-menu-item>
28-
<contextual-menu-item key="save-as-2" text="Save As 2" (click)="onSaveAsSecondClicked()"></contextual-menu-item>
29-
</contextual-menu-item>
30-
</fab-command-bar-item>
31-
<fab-command-bar-item key="copy" text="Copy" [iconProps]="{ iconName: 'Copy' }" (click)="onCopyClicked()"></fab-command-bar-item>
35+
<fab-command-bar-item key="copy1" text="Copy1" [iconProps]="{ iconName: 'Copy' }" (click)="onCopyClicked()"></fab-command-bar-item>
36+
<fab-command-bar-item key="copy2" text="Copy2" [iconProps]="{ iconName: 'Copy' }" (click)="onCopyClicked()"></fab-command-bar-item>
37+
<fab-command-bar-item key="copy3" text="Copy3" [iconProps]="{ iconName: 'Copy' }" (click)="onCopyClicked()"></fab-command-bar-item>
38+
<fab-command-bar-item key="copy4" text="Copy4" [iconProps]="{ iconName: 'Copy' }" (click)="onCopyClicked()"></fab-command-bar-item>
39+
<fab-command-bar-item key="copy5" text="Copy5" [iconProps]="{ iconName: 'Copy' }" (click)="onCopyClicked()"></fab-command-bar-item>
40+
<fab-command-bar-item key="copy6" text="Copy6" [iconProps]="{ iconName: 'Copy' }" (click)="onCopyClicked()"></fab-command-bar-item>
41+
<fab-command-bar-item key="copy7" text="Copy7" [iconProps]="{ iconName: 'Copy' }" (click)="onCopyClicked()"></fab-command-bar-item>
42+
<fab-command-bar-item key="copy8" text="Copy8" [iconProps]="{ iconName: 'Copy' }" (click)="onCopyClicked()"></fab-command-bar-item>
43+
<fab-command-bar-item key="copy9" text="Copy9" [iconProps]="{ iconName: 'Copy' }" (click)="onCopyClicked()"></fab-command-bar-item>
3244
<fab-command-bar-item key="custom" text="custom text" (click)="onCopyClicked()">
3345
<render>
3446
<ng-template let-item="item">
@@ -37,10 +49,10 @@ <h2>Getting up and running...</h2>
3749
</render>
3850

3951
<!-- <render-icon>
40-
<ng-template let-contextualMenuItemProps="contextualMenuItemProps">
41-
<div>custom icon</div>
42-
</ng-template>
43-
</render-icon> -->
52+
<ng-template let-contextualMenuItemProps="contextualMenuItemProps">
53+
<div>custom icon</div>
54+
</ng-template>
55+
</render-icon> -->
4456
</fab-command-bar-item>
4557
<fab-command-bar-item *ngIf="runDisabled" key="sometimesVisible" text="woosh"></fab-command-bar-item>
4658
</items>
@@ -50,9 +62,4 @@ <h2>Getting up and running...</h2>
5062
<fab-command-bar-item key="full-screen" [iconOnly]="true" [iconProps]="{ iconName: fullScreenIcon }" (click)="toggleFullScreen()"></fab-command-bar-item>
5163
</far-items>
5264
</fab-command-bar>
53-
54-
<fab-default-button (onClick)="toggleRun()" text="Toggle run"></fab-default-button>
55-
56-
<fab-panel [isOpen]="isPanelOpen" (onDismiss)="isPanelOpen = false">
57-
</fab-panel>
5865
</div>

libs/core/src/lib/components/wrapper-component.ts

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,25 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33

4-
import { AfterViewInit, ChangeDetectorRef, ComponentFactoryResolver, ComponentRef, ElementRef, Injector, Input, OnChanges, Renderer2, SimpleChanges, TemplateRef, Type } from '@angular/core';
4+
import {
5+
AfterViewInit,
6+
ChangeDetectorRef,
7+
ComponentFactoryResolver,
8+
ComponentRef,
9+
ElementRef,
10+
Injector,
11+
Input,
12+
OnChanges,
13+
Renderer2,
14+
SimpleChanges,
15+
TemplateRef,
16+
Type,
17+
} from '@angular/core';
518
import toStyle from 'css-to-style';
619
import { ReactContentProps } from '../renderer/react-content';
720
import { isReactNode } from '../renderer/react-node';
821
import { isReactRendererData } from '../renderer/renderer';
9-
import { renderComponent, renderFunc, renderTemplate } from '../renderer/renderprop-helpers';
22+
import { createComponentRenderer, createHtmlRenderer, createTemplateRenderer } from '../renderer/renderprop-helpers';
1023
import { afterRenderFinished } from '../utils/render/render-delay';
1124
import { unreachable } from '../utils/types/unreachable';
1225

@@ -142,23 +155,27 @@ export abstract class ReactWrapperComponent<TProps extends {}> implements AfterV
142155
}
143156

144157
if (input instanceof TemplateRef) {
145-
return (context: TContext) => renderTemplate(input, context, additionalProps);
158+
const templateRenderer = createTemplateRenderer(input, additionalProps);
159+
return (context: TContext) => templateRenderer.render(context);
146160
}
147161

148162
if (input instanceof ComponentRef) {
149-
return (context: TContext) => renderComponent(input, context, additionalProps);
163+
const componentRenderer = createComponentRenderer(input, additionalProps);
164+
return (context: TContext) => componentRenderer.render(context);
150165
}
151166

152167
if (input instanceof Function) {
153-
return (context: TContext) => renderFunc(input, context, additionalProps);
168+
const htmlRenderer = createHtmlRenderer(input, additionalProps);
169+
return (context: TContext) => htmlRenderer.render(context);
154170
}
155171

156172
if (typeof input === 'object') {
157173
const { componentType, factoryResolver, injector } = input;
158174
const componentFactory = factoryResolver.resolveComponentFactory(componentType);
159175
const componentRef = componentFactory.create(injector);
160176

161-
return (context: TContext) => renderComponent(componentRef, context, additionalProps);
177+
// Call the function again with the created ComponentRef<TContext>
178+
return this.createInputJsxRenderer(componentRef, additionalProps);
162179
}
163180

164181
unreachable(input);

libs/core/src/lib/renderer/react-content.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import * as React from 'react';
55
import * as ReactDOM from 'react-dom';
66
import { Omit } from '../declarations/omit';
7+
import * as dom from '../utils/dom';
78

89
const DEBUG = false;
910
export const CHILDREN_TO_APPEND_PROP = 'children-to-append';
@@ -46,7 +47,11 @@ export class ReactContent extends React.PureComponent<AllReactContentProps> {
4647
}
4748

4849
const hostElement = this.props.legacyRenderMode ? element : element.parentElement;
49-
this.props[CHILDREN_TO_APPEND_PROP].forEach(child => hostElement.appendChild(child));
50+
51+
// Only add children not already in the DOM
52+
this.props[CHILDREN_TO_APPEND_PROP].filter(child => !dom.isNodeInDOM(child)).forEach(child =>
53+
hostElement.appendChild(child)
54+
);
5055
}
5156
}
5257

libs/core/src/lib/renderer/renderer.ts

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33

44
import { Injectable, Renderer2, RendererStyleFlags2, RendererType2 } from '@angular/core';
55
import { EventManager, ɵDomRendererFactory2, ɵDomSharedStylesHost } from '@angular/platform-browser';
6-
import * as ReactDOM from 'react-dom';
76
import { StringMap } from '../declarations/StringMap';
87
import { isReactNode, ReactNode } from './react-node';
98

@@ -58,17 +57,6 @@ export class AngularReactRendererFactory extends ɵDomRendererFactory2 {
5857
// earlier (as is done for DOM elements) because React element props
5958
// are ReadOnly.
6059

61-
// Workaround for ReactNodes inside ReactContent being added to the root of the VDOM and not removed from the VDOM when unmounted from the DOM.
62-
this.reactRootNodes.forEach(node => {
63-
if (
64-
!isReactNode(node.parent) &&
65-
!document.body.contains(node.parent) &&
66-
ReactDOM.unmountComponentAtNode(node.parent)
67-
) {
68-
this.reactRootNodes.delete(node);
69-
}
70-
});
71-
7260
if (this.isRenderPending) {
7361
// Remove root nodes that are pending destroy after render.
7462
this.reactRootNodes = new Set(Array.from(this.reactRootNodes).filter(node => !node.render().destroyPending));
Lines changed: 46 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33

4-
import { ComponentRef, TemplateRef } from '@angular/core';
4+
import { ComponentRef, EmbeddedViewRef, TemplateRef } from '@angular/core';
55
import * as React from 'react';
66
import { CHILDREN_TO_APPEND_PROP, ReactContent, ReactContentProps } from '../renderer/react-content';
77

8+
export interface RenderPropContext<TContext extends object> {
9+
readonly render: (context: TContext) => JSX.Element;
10+
}
11+
812
function renderReactContent(rootNodes: HTMLElement[], additionalProps?: ReactContentProps): JSX.Element {
913
return React.createElement(ReactContent, {
1014
...additionalProps,
@@ -16,51 +20,71 @@ function renderReactContent(rootNodes: HTMLElement[], additionalProps?: ReactCon
1620
* Wrap a `TemplateRef` with a `JSX.Element`.
1721
*
1822
* @param templateRef The template to wrap
19-
* @param context The context to pass to the template
2023
* @param additionalProps optional additional props to pass to the `ReactContent` object that will render the content.
2124
*/
22-
export function renderTemplate<TContext extends object>(
25+
export function createTemplateRenderer<TContext extends object>(
2326
templateRef: TemplateRef<TContext>,
24-
context?: TContext,
2527
additionalProps?: ReactContentProps
26-
): JSX.Element {
27-
const viewRef = templateRef.createEmbeddedView(context);
28-
viewRef.detectChanges();
28+
): RenderPropContext<TContext> {
29+
let viewRef: EmbeddedViewRef<TContext> | null = null;
30+
let renderedJsx: JSX.Element | null = null;
31+
32+
return {
33+
render: (context: TContext) => {
34+
if (!viewRef) {
35+
viewRef = templateRef.createEmbeddedView(context);
36+
renderedJsx = renderReactContent(viewRef.rootNodes, additionalProps);
37+
} else {
38+
// Mutate the template's context
39+
Object.assign(viewRef.context, context);
40+
}
41+
viewRef.detectChanges();
2942

30-
return renderReactContent(viewRef.rootNodes, additionalProps);
43+
return renderedJsx;
44+
},
45+
};
3146
}
3247

3348
/**
3449
* Wrap a function resolving to an `HTMLElement` with a `JSX.Element`.
3550
*
3651
* @param htmlRenderFunc The function to wrap
37-
* @param context The context to pass to the function
3852
* @param additionalProps optional additional props to pass to the `ReactContent` object that will render the content.
3953
*/
40-
export function renderFunc<TContext extends object>(
54+
export function createHtmlRenderer<TContext extends object>(
4155
htmlRenderFunc: (context: TContext) => HTMLElement,
42-
context?: TContext,
4356
additionalProps?: ReactContentProps
44-
): JSX.Element {
45-
const rootHtmlElement = htmlRenderFunc(context);
46-
47-
return renderReactContent([rootHtmlElement], additionalProps);
57+
): RenderPropContext<TContext> {
58+
return {
59+
render: context => {
60+
const rootHtmlElement = htmlRenderFunc(context);
61+
return renderReactContent([rootHtmlElement], additionalProps);
62+
},
63+
};
4864
}
4965

5066
/**
5167
* Wrap a `ComponentRef` with a `JSX.Element`.
5268
*
5369
* @param htmlRenderFunc The component reference to wrap
54-
* @param context The context to pass to the component as `@Input`
5570
* @param additionalProps optional additional props to pass to the `ReactContent` object that will render the content.
5671
*/
57-
export function renderComponent<TContext extends object>(
72+
export function createComponentRenderer<TContext extends object>(
5873
componentRef: ComponentRef<TContext>,
59-
context?: TContext,
6074
additionalProps?: ReactContentProps
61-
): JSX.Element {
62-
Object.assign(componentRef.instance, context);
63-
componentRef.changeDetectorRef.detectChanges();
75+
): RenderPropContext<TContext> {
76+
let renderedJsx: JSX.Element | null = null;
77+
78+
return {
79+
render: context => {
80+
if (!renderedJsx) {
81+
renderedJsx = renderReactContent([componentRef.location.nativeElement], additionalProps);
82+
}
83+
84+
Object.assign(componentRef.instance, context);
85+
componentRef.changeDetectorRef.detectChanges();
6486

65-
return renderReactContent([componentRef.location.nativeElement], additionalProps);
87+
return renderedJsx;
88+
},
89+
};
6690
}
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
/**
2+
* Checks if a node is in the DOM.
3+
*
4+
* @param node The node to check
5+
* @returns whether the node is in the DOM
6+
*/
7+
export const isNodeInDOM = (node: Node) => node.isConnected || document.body.contains(node);
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
export * from './dom-utils';

0 commit comments

Comments
 (0)