Skip to content

Commit 9165ff2

Browse files
alan-agius4thePunderWoman
authored andcommitted
fix(platform-browser): reuse server generated component styles (#48253)
Prior to this change component styles generated on the server where removed prior to the client side component being rendered and attached it's own styles. In some cases this caused flickering. To mitigate this `initialNavigation: enabledBlocking'` was introduced which allowed the remove of server styles to be defer to a latter stage when the application has finished initialization. This commit changes the need for this, by not removing the server generated component styles and reuse them for client side rendering. PR Close #48253
1 parent 0e5f9ba commit 9165ff2

File tree

11 files changed

+100
-122
lines changed

11 files changed

+100
-122
lines changed

goldens/size-tracking/integration-payloads.json

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
"cli-hello-world": {
33
"uncompressed": {
44
"runtime": 908,
5-
"main": 126191,
5+
"main": 126848,
66
"polyfills": 33792
77
}
88
},
@@ -19,36 +19,36 @@
1919
"cli-hello-world-ivy-i18n": {
2020
"uncompressed": {
2121
"runtime": 926,
22-
"main": 125031,
22+
"main": 125546,
2323
"polyfills": 34676
2424
}
2525
},
2626
"cli-hello-world-lazy": {
2727
"uncompressed": {
2828
"runtime": 2734,
29-
"main": 229515,
29+
"main": 230728,
3030
"polyfills": 33810,
3131
"src_app_lazy_lazy_routes_ts": 487
3232
}
3333
},
3434
"forms": {
3535
"uncompressed": {
3636
"runtime": 888,
37-
"main": 158506,
37+
"main": 159074,
3838
"polyfills": 33772
3939
}
4040
},
4141
"animations": {
4242
"uncompressed": {
4343
"runtime": 898,
44-
"main": 157747,
44+
"main": 158262,
4545
"polyfills": 33782
4646
}
4747
},
4848
"standalone-bootstrap": {
4949
"uncompressed": {
5050
"runtime": 918,
51-
"main": 85542,
51+
"main": 86351,
5252
"polyfills": 33802
5353
}
5454
},
@@ -61,4 +61,4 @@
6161
"bundle": 1214857
6262
}
6363
}
64-
}
64+
}

integration/platform-server/e2e/helloworld-spec.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,21 +15,21 @@ describe('Hello world E2E Tests', function() {
1515
// Load the page without waiting for Angular since it is not bootstrapped automatically.
1616
browser.driver.get(browser.baseUrl + 'helloworld');
1717

18-
const style = browser.driver.findElement(by.css('style[ng-transition="hlw"]'));
18+
const style = browser.driver.findElement(by.css('style[ng-app="hlw"]'));
1919
expect(style.getText()).not.toBeNull();
2020

2121
// Test the contents from the server.
2222
const serverDiv = browser.driver.findElement(by.css('div'));
2323
expect(serverDiv.getText()).toEqual('Hello world!');
24-
2524
// Bootstrap the client side app.
2625
browser.executeScript('doBootstrap()');
2726

2827
// Retest the contents after the client bootstraps.
2928
expect(element(by.css('div')).getText()).toEqual('Hello world!');
3029

31-
// Make sure the server styles got replaced by client side ones.
32-
expect(element(by.css('style[ng-transition="hlw"]')).isPresent()).toBe(false);
30+
// Make sure the server styles get reused by the client.
31+
expect(element(by.css('style[ng-app="hlw"]')).isPresent()).toBeFalsy();
32+
expect(element(by.css('style[ng-style-reused]')).isPresent()).toBeTruthy()
3333
expect(element(by.css('style')).getText()).toBe('');
3434

3535
// Make sure there were no client side errors.

packages/core/test/acceptance/renderer_factory_spec.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -321,8 +321,9 @@ describe('animation renderer factory', () => {
321321
function getRendererFactory2(document: any): RendererFactory2 {
322322
const fakeNgZone: NgZone = new NoopNgZone();
323323
const eventManager = new EventManager([], fakeNgZone);
324+
const appId = 'app-id';
324325
const rendererFactory = new ServerRendererFactory2(
325-
eventManager, fakeNgZone, document, new ɵDomSharedStylesHost(document));
326+
eventManager, fakeNgZone, document, new ɵDomSharedStylesHost(document, appId), appId);
326327
const origCreateRenderer = rendererFactory.createRenderer;
327328
rendererFactory.createRenderer = function(element: any, type: RendererType2|null) {
328329
const renderer = origCreateRenderer.call(this, element, type);

packages/core/test/render3/imported_renderer2.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,9 @@ export class SimpleDomEventsPlugin extends EventManagerPlugin {
3838
export function getRendererFactory2(document: any): RendererFactory2 {
3939
const fakeNgZone: NgZone = new NoopNgZone();
4040
const eventManager = new EventManager([new SimpleDomEventsPlugin(document)], fakeNgZone);
41+
const appId = 'appid';
4142
const rendererFactory = new ɵDomRendererFactory2(
42-
eventManager, new ɵDomSharedStylesHost(document), 'dummyappid', true);
43+
eventManager, new ɵDomSharedStylesHost(document, appId), appId, true);
4344
const origCreateRenderer = rendererFactory.createRenderer;
4445
rendererFactory.createRenderer = function(element: any, type: RendererType2|null) {
4546
const renderer = origCreateRenderer.call(this, element, type);

packages/platform-browser/src/browser.ts

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {CommonModule, DOCUMENT, XhrFactory, ɵPLATFORM_BROWSER_ID as PLATFORM_BR
1010
import {APP_ID, ApplicationConfig as ApplicationConfigFromCore, ApplicationModule, ApplicationRef, createPlatformFactory, ErrorHandler, Inject, InjectionToken, ModuleWithProviders, NgModule, NgZone, Optional, PLATFORM_ID, PLATFORM_INITIALIZER, platformCore, PlatformRef, Provider, RendererFactory2, SkipSelf, StaticProvider, Testability, TestabilityRegistry, Type, ɵINJECTOR_SCOPE as INJECTOR_SCOPE, ɵinternalCreateApplication as internalCreateApplication, ɵsetDocument, ɵTESTABILITY as TESTABILITY, ɵTESTABILITY_GETTER as TESTABILITY_GETTER} from '@angular/core';
1111

1212
import {BrowserDomAdapter} from './browser/browser_adapter';
13-
import {SERVER_TRANSITION_PROVIDERS, TRANSITION_ID} from './browser/server-transition';
13+
import {TRANSITION_ID} from './browser/server-transition';
1414
import {BrowserGetTestability} from './browser/testability';
1515
import {BrowserXhr} from './browser/xhr';
1616
import {DomRendererFactory2, REMOVE_STYLES_ON_COMPONENT_DESTROY} from './dom/dom_renderer';
@@ -211,9 +211,7 @@ const BROWSER_MODULE_PROVIDERS: Provider[] = [
211211
deps: [EventManager, DomSharedStylesHost, APP_ID, REMOVE_STYLES_ON_COMPONENT_DESTROY]
212212
},
213213
{provide: RendererFactory2, useExisting: DomRendererFactory2},
214-
{provide: SharedStylesHost, useExisting: DomSharedStylesHost},
215-
{provide: DomSharedStylesHost, useClass: DomSharedStylesHost, deps: [DOCUMENT]},
216-
{provide: EventManager, useClass: EventManager, deps: [EVENT_MANAGER_PLUGINS, NgZone]},
214+
{provide: SharedStylesHost, useExisting: DomSharedStylesHost}, DomSharedStylesHost, EventManager,
217215
{provide: XhrFactory, useClass: BrowserXhr, deps: []},
218216
NG_DEV_MODE ? {provide: BROWSER_MODULE_PROVIDERS_MARKER, useValue: true} : []
219217
];
@@ -228,10 +226,7 @@ const BROWSER_MODULE_PROVIDERS: Provider[] = [
228226
* @publicApi
229227
*/
230228
@NgModule({
231-
providers: [
232-
...BROWSER_MODULE_PROVIDERS, //
233-
...TESTABILITY_PROVIDERS
234-
],
229+
providers: [...BROWSER_MODULE_PROVIDERS, ...TESTABILITY_PROVIDERS],
235230
exports: [CommonModule, ApplicationModule],
236231
})
237232
export class BrowserModule {
@@ -258,7 +253,6 @@ export class BrowserModule {
258253
providers: [
259254
{provide: APP_ID, useValue: params.appId},
260255
{provide: TRANSITION_ID, useExisting: APP_ID},
261-
SERVER_TRANSITION_PROVIDERS,
262256
],
263257
};
264258
}

packages/platform-browser/src/browser/server-transition.ts

Lines changed: 1 addition & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -6,35 +6,10 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import {DOCUMENT, ɵgetDOM as getDOM} from '@angular/common';
10-
import {APP_INITIALIZER, ApplicationInitStatus, InjectionToken, Injector, StaticProvider} from '@angular/core';
9+
import {InjectionToken} from '@angular/core';
1110

1211
/**
1312
* An id that identifies a particular application being bootstrapped, that should
1413
* match across the client/server boundary.
1514
*/
1615
export const TRANSITION_ID = new InjectionToken('TRANSITION_ID');
17-
18-
export function appInitializerFactory(transitionId: string, document: any, injector: Injector) {
19-
return () => {
20-
// Wait for all application initializers to be completed before removing the styles set by
21-
// the server.
22-
injector.get(ApplicationInitStatus).donePromise.then(() => {
23-
const dom = getDOM();
24-
const styles: HTMLCollectionOf<HTMLStyleElement> =
25-
document.querySelectorAll(`style[ng-transition="${transitionId}"]`);
26-
for (let i = 0; i < styles.length; i++) {
27-
dom.remove(styles[i]);
28-
}
29-
});
30-
};
31-
}
32-
33-
export const SERVER_TRANSITION_PROVIDERS: StaticProvider[] = [
34-
{
35-
provide: APP_INITIALIZER,
36-
useFactory: appInitializerFactory,
37-
deps: [TRANSITION_ID, DOCUMENT, Injector],
38-
multi: true
39-
},
40-
];

packages/platform-browser/src/dom/shared_styles_host.ts

Lines changed: 58 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
*/
88

99
import {DOCUMENT} from '@angular/common';
10-
import {Inject, Injectable, OnDestroy} from '@angular/core';
10+
import {APP_ID, Inject, Injectable, OnDestroy} from '@angular/core';
1111

1212
@Injectable()
1313
export class SharedStylesHost implements OnDestroy {
@@ -69,9 +69,12 @@ export class DomSharedStylesHost extends SharedStylesHost implements OnDestroy {
6969
// Maps all registered host nodes to a list of style nodes that have been added to the host node.
7070
private readonly styleRef = new Map<string, HTMLStyleElement[]>();
7171
private hostNodes = new Set<Node>();
72+
private styleNodesInDOM: Map<string, HTMLStyleElement>|null;
7273

73-
constructor(@Inject(DOCUMENT) private readonly doc: any) {
74+
constructor(
75+
@Inject(DOCUMENT) private readonly doc: Document, @Inject(APP_ID) private appId: string) {
7476
super();
77+
this.styleNodesInDOM = this.collectServerRenderedStyles();
7578
this.resetHostNodes();
7679
}
7780

@@ -84,14 +87,20 @@ export class DomSharedStylesHost extends SharedStylesHost implements OnDestroy {
8487
override onStyleRemoved(style: string): void {
8588
const styleRef = this.styleRef;
8689
const styleElements = styleRef.get(style);
87-
styleElements?.forEach(e => e.remove());
90+
styleElements?.forEach(node => node.remove());
8891
styleRef.delete(style);
8992
}
9093

9194
override ngOnDestroy(): void {
9295
super.ngOnDestroy();
9396
this.styleRef.clear();
9497
this.resetHostNodes();
98+
99+
const styleNodesInDOM = this.styleNodesInDOM;
100+
if (styleNodesInDOM) {
101+
styleNodesInDOM.forEach(node => node.remove());
102+
styleNodesInDOM.clear();
103+
}
95104
}
96105

97106
addHost(hostNode: Node): void {
@@ -106,16 +115,58 @@ export class DomSharedStylesHost extends SharedStylesHost implements OnDestroy {
106115
this.hostNodes.delete(hostNode);
107116
}
108117

118+
private collectServerRenderedStyles(): Map<string, HTMLStyleElement>|null {
119+
const styles =
120+
this.doc.head?.querySelectorAll<HTMLStyleElement>(`style[ng-app="${this.appId}"]`);
121+
122+
if (styles?.length) {
123+
const styleMap = new Map<string, HTMLStyleElement>();
124+
125+
styles.forEach(style => {
126+
if (style.textContent != null) {
127+
styleMap.set(style.textContent, style);
128+
}
129+
});
130+
131+
return styleMap;
132+
}
133+
134+
return null;
135+
}
136+
137+
private getStyleElement(host: Node, style: string): HTMLStyleElement {
138+
const styleNodesInDOM = this.styleNodesInDOM;
139+
const styleEl = styleNodesInDOM?.get(style);
140+
if (styleEl?.parentNode === host) {
141+
// `styleNodesInDOM` cannot be undefined due to the above `styleNodesInDOM?.get`.
142+
styleNodesInDOM!.delete(style);
143+
styleEl.removeAttribute('ng-app');
144+
145+
if (typeof ngDevMode === 'undefined' || ngDevMode) {
146+
// This attribute is solely used for debugging purposes.
147+
styleEl.setAttribute('ng-style-reused', '');
148+
}
149+
150+
return styleEl;
151+
} else {
152+
const styleEl = this.doc.createElement('style');
153+
styleEl.textContent = style;
154+
155+
return styleEl;
156+
}
157+
}
158+
109159
private addStyleToHost(host: Node, style: string): void {
110-
const styleEl = this.doc.createElement('style');
111-
styleEl.textContent = style;
160+
const styleEl = this.getStyleElement(host, style);
161+
112162
host.appendChild(styleEl);
113163

114-
const styleElRef = this.styleRef.get(style);
164+
const styleRef = this.styleRef;
165+
const styleElRef = styleRef.get(style);
115166
if (styleElRef) {
116167
styleElRef.push(styleEl);
117168
} else {
118-
this.styleRef.set(style, [styleEl]);
169+
styleRef.set(style, [styleEl]);
119170
}
120171
}
121172

packages/platform-browser/test/browser/bootstrap_spec.ts

Lines changed: 0 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -628,43 +628,6 @@ function bootstrap(
628628
})();
629629
});
630630

631-
it('should remove styles when transitioning from a server render', done => {
632-
@Component({
633-
selector: 'root',
634-
template: 'root',
635-
})
636-
class RootCmp {
637-
}
638-
639-
@NgModule({
640-
bootstrap: [RootCmp],
641-
declarations: [RootCmp],
642-
imports: [BrowserModule.withServerTransition({appId: 'my-app'})],
643-
})
644-
class TestModule {
645-
}
646-
647-
// First, set up styles to be removed.
648-
const dom = getDOM();
649-
const platform = platformBrowserDynamic();
650-
const document = platform.injector.get(DOCUMENT);
651-
const style = dom.createElement('style', document);
652-
style.setAttribute('ng-transition', 'my-app');
653-
document.head.appendChild(style);
654-
655-
const root = dom.createElement('root', document);
656-
document.body.appendChild(root);
657-
658-
platform.bootstrapModule(TestModule).then(() => {
659-
const styles: HTMLElement[] =
660-
Array.prototype.slice.apply(document.getElementsByTagName('style') || []);
661-
styles.forEach(style => {
662-
expect(style.getAttribute('ng-transition')).not.toBe('my-app');
663-
});
664-
done();
665-
}, done.fail);
666-
});
667-
668631
it('should register each application with the testability registry', async () => {
669632
const ngModuleRef1: NgModuleRef<unknown> = await bootstrap(HelloRootCmp, testProviders);
670633
const ngModuleRef2: NgModuleRef<unknown> = await bootstrap(HelloRootCmp2, testProviders);

packages/platform-browser/test/dom/shared_styles_host_spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import {expect} from '@angular/platform-browser/testing/src/matchers';
1818
beforeEach(() => {
1919
doc = getDOM().createHtmlDocument();
2020
doc.title = '';
21-
ssh = new DomSharedStylesHost(doc);
21+
ssh = new DomSharedStylesHost(doc, 'app-id');
2222
someHost = getDOM().createElement('div');
2323
});
2424

0 commit comments

Comments
 (0)