Skip to content

Commit faee5c6

Browse files
alan-agius4alan-agius4
committed
refactor: add TypeScript declaration merging for Critters
This refactor adds a workaround to enable TypeScript declaration merging for the Critters class. We initially tried using interface merging on the default-exported Critters class: ```typescript interface Critters { embedLinkedStylesheet(link: PartialHTMLElement, document: PartialDocument): Promise<unknown>; } ``` However, since Critters is exported as a default class, TypeScript's declaration merging does not apply. To solve this, we introduced a new class, CrittersBase, which extends Critters, and added the required method in the CrittersBase interface: ```typescript interface CrittersBase { embedLinkedStylesheet(link: PartialHTMLElement, document: PartialDocument): Promise<unknown>; } class CrittersBase extends Critters {} ```
1 parent 607a97c commit faee5c6

File tree

4 files changed

+114
-116
lines changed

4 files changed

+114
-116
lines changed

‎packages/angular/build/src/utils/index-file/inline-critical-css.ts

Lines changed: 52 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -20,36 +20,46 @@ const MEDIA_SET_HANDLER_PATTERN = /^this\.media=["'](.*)["'];?$/;
2020
const CSP_MEDIA_ATTR = 'ngCspMedia';
2121

2222
/**
23-
* Script text used to change the media value of the link tags.
23+
* Script that dynamically updates the `media` attribute of `<link>` tags based on a custom attribute (`CSP_MEDIA_ATTR`).
2424
*
2525
* NOTE:
2626
* We do not use `document.querySelectorAll('link').forEach((s) => s.addEventListener('load', ...)`
27-
* because this does not always fire on Chome.
27+
* because load events are not always triggered reliably on Chrome.
2828
* See: https://github.com/angular/angular-cli/issues/26932 and https://crbug.com/1521256
29+
*
30+
* The script:
31+
* - Ensures the event target is a `<link>` tag with the `CSP_MEDIA_ATTR` attribute.
32+
* - Updates the `media` attribute with the value of `CSP_MEDIA_ATTR` and then removes the attribute.
33+
* - Removes the event listener when all relevant `<link>` tags have been processed.
34+
* - Uses event capturing (the `true` parameter) since load events do not bubble up the DOM.
2935
*/
30-
const LINK_LOAD_SCRIPT_CONTENT = [
31-
'(() => {',
32-
` const CSP_MEDIA_ATTR = '${CSP_MEDIA_ATTR}';`,
33-
' const documentElement = document.documentElement;',
34-
' const listener = (e) => {',
35-
' const target = e.target;',
36-
` if (!target || target.tagName !== 'LINK' || !target.hasAttribute(CSP_MEDIA_ATTR)) {`,
37-
' return;',
38-
' }',
39-
40-
' target.media = target.getAttribute(CSP_MEDIA_ATTR);',
41-
' target.removeAttribute(CSP_MEDIA_ATTR);',
42-
43-
// Remove onload listener when there are no longer styles that need to be loaded.
44-
' if (!document.head.querySelector(`link[${CSP_MEDIA_ATTR}]`)) {',
45-
` documentElement.removeEventListener('load', listener);`,
46-
' }',
47-
' };',
48-
49-
// We use an event with capturing (the true parameter) because load events don't bubble.
50-
` documentElement.addEventListener('load', listener, true);`,
51-
'})();',
52-
].join('\n');
36+
const LINK_LOAD_SCRIPT_CONTENT = `
37+
(() => {
38+
const CSP_MEDIA_ATTR = '${CSP_MEDIA_ATTR}';
39+
const documentElement = document.documentElement;
40+
41+
// Listener for load events on link tags.
42+
const listener = (e) => {
43+
const target = e.target;
44+
if (
45+
!target ||
46+
target.tagName !== 'LINK' ||
47+
!target.hasAttribute(CSP_MEDIA_ATTR)
48+
) {
49+
return;
50+
}
51+
52+
target.media = target.getAttribute(CSP_MEDIA_ATTR);
53+
target.removeAttribute(CSP_MEDIA_ATTR);
54+
55+
if (!document.head.querySelector(\`link[\${CSP_MEDIA_ATTR}]\`)) {
56+
documentElement.removeEventListener('load', listener);
57+
}
58+
};
59+
60+
documentElement.addEventListener('load', listener, true);
61+
})();
62+
`.trim();
5363

5464
export interface InlineCriticalCssProcessOptions {
5565
outputPath: string;
@@ -85,22 +95,22 @@ interface PartialDocument {
8595
querySelector(selector: string): PartialHTMLElement | null;
8696
}
8797

88-
/** Signature of the `Critters.embedLinkedStylesheet` method. */
89-
type EmbedLinkedStylesheetFn = (
90-
link: PartialHTMLElement,
91-
document: PartialDocument,
92-
) => Promise<unknown>;
98+
/* eslint-disable @typescript-eslint/no-unsafe-declaration-merging */
9399

94-
class CrittersExtended extends Critters {
100+
// We use Typescript declaration merging because `embedLinkedStylesheet` it's not declared in
101+
// the `Critters` types which means that we can't call the `super` implementation.
102+
interface CrittersBase {
103+
embedLinkedStylesheet(link: PartialHTMLElement, document: PartialDocument): Promise<unknown>;
104+
}
105+
class CrittersBase extends Critters {}
106+
/* eslint-enable @typescript-eslint/no-unsafe-declaration-merging */
107+
108+
class CrittersExtended extends CrittersBase {
95109
readonly warnings: string[] = [];
96110
readonly errors: string[] = [];
97-
private initialEmbedLinkedStylesheet: EmbedLinkedStylesheetFn;
98111
private addedCspScriptsDocuments = new WeakSet<PartialDocument>();
99112
private documentNonces = new WeakMap<PartialDocument, string | null>();
100113

101-
// Inherited from `Critters`, but not exposed in the typings.
102-
protected declare embedLinkedStylesheet: EmbedLinkedStylesheetFn;
103-
104114
constructor(
105115
private readonly optionsExtended: InlineCriticalCssProcessorOptions &
106116
InlineCriticalCssProcessOptions,
@@ -119,17 +129,11 @@ class CrittersExtended extends Critters {
119129
reduceInlineStyles: false,
120130
mergeStylesheets: false,
121131
// Note: if `preload` changes to anything other than `media`, the logic in
122-
// `embedLinkedStylesheetOverride` will have to be updated.
132+
// `embedLinkedStylesheet` will have to be updated.
123133
preload: 'media',
124134
noscriptFallback: true,
125135
inlineFonts: true,
126136
});
127-
128-
// We can't use inheritance to override `embedLinkedStylesheet`, because it's not declared in
129-
// the `Critters` .d.ts which means that we can't call the `super` implementation. TS doesn't
130-
// allow for `super` to be cast to a different type.
131-
this.initialEmbedLinkedStylesheet = this.embedLinkedStylesheet;
132-
this.embedLinkedStylesheet = this.embedLinkedStylesheetOverride;
133137
}
134138

135139
public override readFile(path: string): Promise<string> {
@@ -142,7 +146,10 @@ class CrittersExtended extends Critters {
142146
* Override of the Critters `embedLinkedStylesheet` method
143147
* that makes it work with Angular's CSP APIs.
144148
*/
145-
private embedLinkedStylesheetOverride: EmbedLinkedStylesheetFn = async (link, document) => {
149+
override async embedLinkedStylesheet(
150+
link: PartialHTMLElement,
151+
document: PartialDocument,
152+
): Promise<unknown> {
146153
if (link.getAttribute('media') === 'print' && link.next?.name === 'noscript') {
147154
// Workaround for https://github.com/GoogleChromeLabs/critters/issues/64
148155
// NB: this is only needed for the webpack based builders.
@@ -154,7 +161,7 @@ class CrittersExtended extends Critters {
154161
}
155162
}
156163

157-
const returnValue = await this.initialEmbedLinkedStylesheet(link, document);
164+
const returnValue = await super.embedLinkedStylesheet(link, document);
158165
const cspNonce = this.findCspNonce(document);
159166

160167
if (cspNonce) {
@@ -182,7 +189,7 @@ class CrittersExtended extends Critters {
182189
}
183190

184191
return returnValue;
185-
};
192+
}
186193

187194
/**
188195
* Finds the CSP nonce for a specific document.

‎packages/angular/ssr/src/app.ts

Lines changed: 4 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -187,34 +187,16 @@ export class AngularServerApp {
187187

188188
if (manifest.inlineCriticalCss) {
189189
// Optionally inline critical CSS.
190-
const inlineCriticalCssProcessor = this.getOrCreateInlineCssProcessor();
191-
html = await inlineCriticalCssProcessor.process(html);
192-
}
193-
194-
return new Response(html, responseInit);
195-
}
196-
197-
/**
198-
* Retrieves or creates the inline critical CSS processor.
199-
* If one does not exist, it initializes a new instance.
200-
*
201-
* @returns The inline critical CSS processor instance.
202-
*/
203-
private getOrCreateInlineCssProcessor(): InlineCriticalCssProcessor {
204-
let inlineCriticalCssProcessor = this.inlineCriticalCssProcessor;
205-
206-
if (!inlineCriticalCssProcessor) {
207-
inlineCriticalCssProcessor = new InlineCriticalCssProcessor();
208-
inlineCriticalCssProcessor.readFile = (path: string) => {
190+
this.inlineCriticalCssProcessor ??= new InlineCriticalCssProcessor((path: string) => {
209191
const fileName = path.split('/').pop() ?? path;
210192

211193
return this.assets.getServerAsset(fileName);
212-
};
194+
});
213195

214-
this.inlineCriticalCssProcessor = inlineCriticalCssProcessor;
196+
html = await this.inlineCriticalCssProcessor.process(html);
215197
}
216198

217-
return inlineCriticalCssProcessor;
199+
return new Response(html, responseInit);
218200
}
219201
}
220202

‎packages/angular/ssr/src/common-engine/inline-css-processor.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,16 +13,15 @@ export class CommonEngineInlineCriticalCssProcessor {
1313
private readonly resourceCache = new Map<string, string>();
1414

1515
async process(html: string, outputPath: string | undefined): Promise<string> {
16-
const critters = new InlineCriticalCssProcessor(outputPath);
17-
critters.readFile = async (path: string): Promise<string> => {
16+
const critters = new InlineCriticalCssProcessor(async (path) => {
1817
let resourceContent = this.resourceCache.get(path);
1918
if (resourceContent === undefined) {
2019
resourceContent = await readFile(path, 'utf-8');
2120
this.resourceCache.set(path, resourceContent);
2221
}
2322

2423
return resourceContent;
25-
};
24+
}, outputPath);
2625

2726
return critters.process(html);
2827
}

‎packages/angular/ssr/src/utils/inline-critical-css.ts

Lines changed: 56 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -19,36 +19,46 @@ const MEDIA_SET_HANDLER_PATTERN = /^this\.media=["'](.*)["'];?$/;
1919
const CSP_MEDIA_ATTR = 'ngCspMedia';
2020

2121
/**
22-
* Script text used to change the media value of the link tags.
22+
* Script that dynamically updates the `media` attribute of `<link>` tags based on a custom attribute (`CSP_MEDIA_ATTR`).
2323
*
2424
* NOTE:
2525
* We do not use `document.querySelectorAll('link').forEach((s) => s.addEventListener('load', ...)`
26-
* because this does not always fire on Chome.
26+
* because load events are not always triggered reliably on Chrome.
2727
* See: https://github.com/angular/angular-cli/issues/26932 and https://crbug.com/1521256
28+
*
29+
* The script:
30+
* - Ensures the event target is a `<link>` tag with the `CSP_MEDIA_ATTR` attribute.
31+
* - Updates the `media` attribute with the value of `CSP_MEDIA_ATTR` and then removes the attribute.
32+
* - Removes the event listener when all relevant `<link>` tags have been processed.
33+
* - Uses event capturing (the `true` parameter) since load events do not bubble up the DOM.
2834
*/
29-
const LINK_LOAD_SCRIPT_CONTENT = [
30-
'(() => {',
31-
` const CSP_MEDIA_ATTR = '${CSP_MEDIA_ATTR}';`,
32-
' const documentElement = document.documentElement;',
33-
' const listener = (e) => {',
34-
' const target = e.target;',
35-
` if (!target || target.tagName !== 'LINK' || !target.hasAttribute(CSP_MEDIA_ATTR)) {`,
36-
' return;',
37-
' }',
38-
39-
' target.media = target.getAttribute(CSP_MEDIA_ATTR);',
40-
' target.removeAttribute(CSP_MEDIA_ATTR);',
41-
42-
// Remove onload listener when there are no longer styles that need to be loaded.
43-
' if (!document.head.querySelector(`link[${CSP_MEDIA_ATTR}]`)) {',
44-
` documentElement.removeEventListener('load', listener);`,
45-
' }',
46-
' };',
47-
48-
// We use an event with capturing (the true parameter) because load events don't bubble.
49-
` documentElement.addEventListener('load', listener, true);`,
50-
'})();',
51-
].join('\n');
35+
const LINK_LOAD_SCRIPT_CONTENT = `
36+
(() => {
37+
const CSP_MEDIA_ATTR = '${CSP_MEDIA_ATTR}';
38+
const documentElement = document.documentElement;
39+
40+
// Listener for load events on link tags.
41+
const listener = (e) => {
42+
const target = e.target;
43+
if (
44+
!target ||
45+
target.tagName !== 'LINK' ||
46+
!target.hasAttribute(CSP_MEDIA_ATTR)
47+
) {
48+
return;
49+
}
50+
51+
target.media = target.getAttribute(CSP_MEDIA_ATTR);
52+
target.removeAttribute(CSP_MEDIA_ATTR);
53+
54+
if (!document.head.querySelector(\`link[\${CSP_MEDIA_ATTR}]\`)) {
55+
documentElement.removeEventListener('load', listener);
56+
}
57+
};
58+
59+
documentElement.addEventListener('load', listener, true);
60+
})();
61+
`.trim();
5262

5363
/** Partial representation of an `HTMLElement`. */
5464
interface PartialHTMLElement {
@@ -74,21 +84,24 @@ interface PartialDocument {
7484
querySelector(selector: string): PartialHTMLElement | null;
7585
}
7686

77-
/** Signature of the `Critters.embedLinkedStylesheet` method. */
78-
type EmbedLinkedStylesheetFn = (
79-
link: PartialHTMLElement,
80-
document: PartialDocument,
81-
) => Promise<unknown>;
87+
/* eslint-disable @typescript-eslint/no-unsafe-declaration-merging */
8288

83-
export class InlineCriticalCssProcessor extends Critters {
84-
private initialEmbedLinkedStylesheet: EmbedLinkedStylesheetFn;
89+
// We use Typescript declaration merging because `embedLinkedStylesheet` it's not declared in
90+
// the `Critters` types which means that we can't call the `super` implementation.
91+
interface CrittersBase {
92+
embedLinkedStylesheet(link: PartialHTMLElement, document: PartialDocument): Promise<unknown>;
93+
}
94+
class CrittersBase extends Critters {}
95+
/* eslint-enable @typescript-eslint/no-unsafe-declaration-merging */
96+
97+
export class InlineCriticalCssProcessor extends CrittersBase {
8598
private addedCspScriptsDocuments = new WeakSet<PartialDocument>();
8699
private documentNonces = new WeakMap<PartialDocument, string | null>();
87100

88-
// Inherited from `Critters`, but not exposed in the typings.
89-
protected declare embedLinkedStylesheet: EmbedLinkedStylesheetFn;
90-
91-
constructor(readonly outputPath?: string) {
101+
constructor(
102+
public override readFile: (path: string) => Promise<string>,
103+
readonly outputPath?: string,
104+
) {
92105
super({
93106
logger: {
94107
// eslint-disable-next-line no-console
@@ -105,24 +118,21 @@ export class InlineCriticalCssProcessor extends Critters {
105118
reduceInlineStyles: false,
106119
mergeStylesheets: false,
107120
// Note: if `preload` changes to anything other than `media`, the logic in
108-
// `embedLinkedStylesheetOverride` will have to be updated.
121+
// `embedLinkedStylesheet` will have to be updated.
109122
preload: 'media',
110123
noscriptFallback: true,
111124
inlineFonts: true,
112125
});
113-
114-
// We can't use inheritance to override `embedLinkedStylesheet`, because it's not declared in
115-
// the `Critters` .d.ts which means that we can't call the `super` implementation. TS doesn't
116-
// allow for `super` to be cast to a different type.
117-
this.initialEmbedLinkedStylesheet = this.embedLinkedStylesheet;
118-
this.embedLinkedStylesheet = this.embedLinkedStylesheetOverride;
119126
}
120127

121128
/**
122129
* Override of the Critters `embedLinkedStylesheet` method
123130
* that makes it work with Angular's CSP APIs.
124131
*/
125-
private embedLinkedStylesheetOverride: EmbedLinkedStylesheetFn = async (link, document) => {
132+
override async embedLinkedStylesheet(
133+
link: PartialHTMLElement,
134+
document: PartialDocument,
135+
): Promise<unknown> {
126136
if (link.getAttribute('media') === 'print' && link.next?.name === 'noscript') {
127137
// Workaround for https://github.com/GoogleChromeLabs/critters/issues/64
128138
// NB: this is only needed for the webpack based builders.
@@ -134,7 +144,7 @@ export class InlineCriticalCssProcessor extends Critters {
134144
}
135145
}
136146

137-
const returnValue = await this.initialEmbedLinkedStylesheet(link, document);
147+
const returnValue = await super.embedLinkedStylesheet(link, document);
138148
const cspNonce = this.findCspNonce(document);
139149

140150
if (cspNonce) {
@@ -162,7 +172,7 @@ export class InlineCriticalCssProcessor extends Critters {
162172
}
163173

164174
return returnValue;
165-
};
175+
}
166176

167177
/**
168178
* Finds the CSP nonce for a specific document.

0 commit comments

Comments
 (0)