Skip to content

Commit

Permalink
fixup! perf(platform-browser): disable styles of removed components i…
Browse files Browse the repository at this point in the history 
…nstead of removing
  • Loading branch information
@alan-agius4
alan-agius4 committed Sep 19, 2023
1 parent 8f0d53d commit 5e9fe7f
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 26 deletions.
18 changes: 11 additions & 7 deletions packages/platform-browser/src/dom/dom_renderer.ts
View file
Expand Up @@ -36,7 +36,7 @@ const REMOVE_STYLES_ON_COMPONENT_DESTROY_DEFAULT = true;

/**
* A [DI token](guide/glossary#di-token "DI token definition") that indicates whether styles
* of destroyed components with `ViewEncapsulation.None` should be disabled.
* of destroyed components should be disabled.
*
* By default, the value is set to `true`.
* @publicApi
Expand Down Expand Up @@ -112,20 +112,22 @@ export class DomRendererFactory2 implements RendererFactory2, OnDestroy {
const ngZone = this.ngZone;
const eventManager = this.eventManager;
const sharedStylesHost = this.sharedStylesHost;
const disableStylesOnCompDestroy = this.disableStylesOnCompDestroy;
const platformIsServer = this.platformIsServer;

switch (type.encapsulation) {
case ViewEncapsulation.Emulated:
renderer = new EmulatedEncapsulationDomRenderer2(
eventManager, sharedStylesHost, type, this.appId, doc, ngZone, platformIsServer);
eventManager, sharedStylesHost, type, this.appId, disableStylesOnCompDestroy, doc,
ngZone, platformIsServer);
break;
case ViewEncapsulation.ShadowDom:
return new ShadowDomRenderer(
eventManager, sharedStylesHost, element, type, doc, ngZone, this.nonce,
platformIsServer);
default:
renderer = new NoneEncapsulationDomRenderer(
eventManager, sharedStylesHost, type, this.disableStylesOnCompDestroy, doc, ngZone,
eventManager, sharedStylesHost, type, disableStylesOnCompDestroy, doc, ngZone,
platformIsServer);
break;
}
Expand Down Expand Up @@ -394,7 +396,6 @@ class ShadowDomRenderer extends DefaultDomRenderer2 {

class NoneEncapsulationDomRenderer extends DefaultDomRenderer2 {
protected styles: string[];

constructor(
eventManager: EventManager,
private readonly sharedStylesHost: SharedStylesHost,
Expand Down Expand Up @@ -425,10 +426,13 @@ class EmulatedEncapsulationDomRenderer2 extends NoneEncapsulationDomRenderer {

constructor(
eventManager: EventManager, sharedStylesHost: SharedStylesHost, component: RendererType2,
appId: string, doc: Document, ngZone: NgZone, platformIsServer: boolean) {
super(eventManager, sharedStylesHost, component, false, doc, ngZone, platformIsServer);
const compId = appId + '-' + component.id;
appId: string, disableStylesOnCompDestroy: boolean, doc: Document, ngZone: NgZone,
platformIsServer: boolean) {
super(
eventManager, sharedStylesHost, component, disableStylesOnCompDestroy, doc, ngZone,
platformIsServer);

const compId = appId + '-' + component.id;
this.styles = shimStylesContent(compId, component.styles);
this.contentAttr = shimContentAttribute(compId);
this.hostAttr = shimHostAttribute(compId);
Expand Down
60 changes: 43 additions & 17 deletions packages/platform-browser/src/dom/shared_styles_host.ts
View file
Expand Up @@ -17,7 +17,7 @@ export class SharedStylesHost implements OnDestroy {
// Maps all registered host nodes to a list of style nodes that have been added to the host node.
private readonly styleRef = new Map < string /** Style string */, {
elements: HTMLStyleElement[];
usage: number
usage: number;
}
> ();
private readonly hostNodes = new Set<Node>();
Expand All @@ -28,7 +28,8 @@ export class SharedStylesHost implements OnDestroy {
@Inject(DOCUMENT) private readonly doc: Document,
@Inject(APP_ID) private readonly appId: string,
@Inject(CSP_NONCE) @Optional() private nonce?: string|null,
@Inject(PLATFORM_ID) readonly platformId: object = {}) {
@Inject(PLATFORM_ID) readonly platformId: object = {},
) {
this.styleNodesInDOM = this.collectServerRenderedStyles();
this.platformIsServer = isPlatformServer(platformId);
this.resetHostNodes();
Expand All @@ -48,10 +49,8 @@ export class SharedStylesHost implements OnDestroy {
for (const style of styles) {
const usageCount = this.changeUsageCount(style, -1);

if (usageCount <= 0) {
this.visitStyleElement(style, (node) => {
node.disabled = true;
});
if (usageCount === 0) {
this.visitStyleElement(style, disableStylesheet);
}
}
}
Expand All @@ -68,9 +67,7 @@ export class SharedStylesHost implements OnDestroy {
}

for (const style of this.getAllStyles()) {
this.visitStyleElement(style, (node) => {
node.remove();
});
this.visitStyleElement(style, (node) => node.remove());
this.styleRef.delete(style);
}

Expand Down Expand Up @@ -101,7 +98,8 @@ export class SharedStylesHost implements OnDestroy {

private collectServerRenderedStyles(): Map<string, HTMLStyleElement>|null {
const styles = this.doc.head?.querySelectorAll<HTMLStyleElement>(
`style[${APP_ID_ATTRIBUTE_NAME}="${this.appId}"]`);
`style[${APP_ID_ATTRIBUTE_NAME}="${this.appId}"]`,
);

if (styles?.length) {
const styleMap = new Map<string, HTMLStyleElement>();
Expand All @@ -122,12 +120,12 @@ export class SharedStylesHost implements OnDestroy {
const map = this.styleRef;
if (map.has(style)) {
const styleRefValue = map.get(style)!;
styleRefValue.usage = Math.max(0, styleRefValue.usage + delta);
styleRefValue.usage = nonNegativeNumber(styleRefValue.usage + delta);

return styleRefValue.usage;
}

const usage = Math.max(0, delta);
const usage = nonNegativeNumber(delta);
map.set(style, {usage, elements: []});
return usage;
}
Expand Down Expand Up @@ -167,14 +165,18 @@ export class SharedStylesHost implements OnDestroy {
private addStyleToHost(host: Node, style: string): void {
const styleEl = this.getStyleElement(host, style);

// Enable the stylesheet
styleEl.disabled = false;
host.appendChild(styleEl);

const styleRef = this.styleRef;
const styleElRef = styleRef.get(style)?.elements;
if (styleElRef) {
styleElRef.push(styleEl);
const styleResult = styleRef.get(style);
if (styleResult) {
if (styleResult.usage === 0) {
disableStylesheet(styleEl);
} else {
enableStylesheet(styleEl);
}

styleResult.elements.push(styleEl);
} else {
styleRef.set(style, {elements: [styleEl], usage: 1});
}
Expand All @@ -187,3 +189,27 @@ export class SharedStylesHost implements OnDestroy {
hostNodes.add(this.doc.head);
}
}

/**
* When a component that has styles is destroyed, we disable stylesheets
* instead of removing them to avoid performance issues related to style
* recalculation in a browser.
*/
function disableStylesheet(node: HTMLStyleElement): void {
node.disabled = true;
}

/**
* Enables a stylesheet in a browser, see the `disableStylesheet` function
* docs for additional info.
*/
function enableStylesheet(node: HTMLStyleElement): void {
node.disabled = false;
}

/**
* When the value is a negative a value of `0` is returned.
*/
function nonNegativeNumber(value: number): number {
return value < 0 ? 0 : value;
}
4 changes: 2 additions & 2 deletions packages/platform-browser/test/dom/dom_renderer_spec.ts
View file
Expand Up @@ -214,7 +214,7 @@ import {expect} from '@angular/platform-browser/testing/src/matchers';
});

describe('When `REMOVE_STYLES_ON_COMPONENT_DESTROY` is `true` or default.', () => {
it('should not disable styles for components with encapsulation emulated', async () => {
it('should disable styles for components with encapsulation emulated', async () => {
const fixture = TestBed.createComponent(SomeAppForCleanUp);
const compInstance = fixture.componentInstance;
compInstance.showEmulatedComponents = true;
Expand All @@ -235,7 +235,7 @@ import {expect} from '@angular/platform-browser/testing/src/matchers';
// Verify style is not disabled in DOM
const styles = await getStyles(fixture, '.emulated');
expect(styles?.length).toBe(1);
expect(styles?.[0].nativeElement.disabled).toBeFalse();
expect(styles?.[0].nativeElement.disabled).toBeTrue();
});

it('should disable styles for components with encapsulation none', async () => {
Expand Down

0 comments on commit 5e9fe7f

Please sign in to comment.