Skip to content

Commit

Permalink
refactor: add TypeScript declaration merging for Critters
Browse files Browse the repository at this point in the history 
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 {}
```
  • Loading branch information
@alan-agius4
alan-agius4 committed Aug 23, 2024
1 parent f7ea0ad commit 5d45ecd
Show file tree
Hide file tree
Showing 4 changed files with 108 additions and 116 deletions.
94 changes: 49 additions & 45 deletions packages/angular/build/src/utils/index-file/inline-critical-css.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,36 +20,46 @@ const MEDIA_SET_HANDLER_PATTERN = /^this\.media=["'](.*)["'];?$/;
const CSP_MEDIA_ATTR = 'ngCspMedia';

/**
* Script text used to change the media value of the link tags.
* Script that dynamically updates the `media` attribute of `<link>` tags based on a custom attribute (`CSP_MEDIA_ATTR`).
*
* NOTE:
* We do not use `document.querySelectorAll('link').forEach((s) => s.addEventListener('load', ...)`
* because this does not always fire on Chome.
* because load events are not always triggered reliably on Chrome.
* See: https://github.com/angular/angular-cli/issues/26932 and https://crbug.com/1521256
*
* The script:
* - Ensures the event target is a `<link>` tag with the `CSP_MEDIA_ATTR` attribute.
* - Updates the `media` attribute with the value of `CSP_MEDIA_ATTR` and then removes the attribute.
* - Removes the event listener when all relevant `<link>` tags have been processed.
* - Uses event capturing (the `true` parameter) since load events do not bubble up the DOM.
*/
const LINK_LOAD_SCRIPT_CONTENT = [
'(() => {',
` const CSP_MEDIA_ATTR = '${CSP_MEDIA_ATTR}';`,
' const documentElement = document.documentElement;',
' const listener = (e) => {',
' const target = e.target;',
` if (!target || target.tagName !== 'LINK' || !target.hasAttribute(CSP_MEDIA_ATTR)) {`,
' return;',
' }',

' target.media = target.getAttribute(CSP_MEDIA_ATTR);',
' target.removeAttribute(CSP_MEDIA_ATTR);',

// Remove onload listener when there are no longer styles that need to be loaded.
' if (!document.head.querySelector(`link[${CSP_MEDIA_ATTR}]`)) {',
` documentElement.removeEventListener('load', listener);`,
' }',
' };',

// We use an event with capturing (the true parameter) because load events don't bubble.
` documentElement.addEventListener('load', listener, true);`,
'})();',
].join('\n');
const LINK_LOAD_SCRIPT_CONTENT = `
(() => {
const CSP_MEDIA_ATTR = '${CSP_MEDIA_ATTR}';
const documentElement = document.documentElement;
// Listener for load events on link tags.
const listener = (e) => {
const target = e.target;
if (
!target ||
target.tagName !== 'LINK' ||
!target.hasAttribute(CSP_MEDIA_ATTR)
) {
return;
}
target.media = target.getAttribute(CSP_MEDIA_ATTR);
target.removeAttribute(CSP_MEDIA_ATTR);
if (!document.head.querySelector(\`link[\${CSP_MEDIA_ATTR}]\`)) {
documentElement.removeEventListener('load', listener);
}
};
documentElement.addEventListener('load', listener, true);
})();
`.trim();

export interface InlineCriticalCssProcessOptions {
outputPath: string;
Expand Down Expand Up @@ -85,22 +95,19 @@ interface PartialDocument {
querySelector(selector: string): PartialHTMLElement | null;
}

/** Signature of the `Critters.embedLinkedStylesheet` method. */
type EmbedLinkedStylesheetFn = (
link: PartialHTMLElement,
document: PartialDocument,
) => Promise<unknown>;
/* eslint-disable @typescript-eslint/no-unsafe-declaration-merging */
interface CrittersBase {
embedLinkedStylesheet(link: PartialHTMLElement, document: PartialDocument): Promise<unknown>;
}
class CrittersBase extends Critters {}
/* eslint-enable @typescript-eslint/no-unsafe-declaration-merging */

class CrittersExtended extends Critters {
class CrittersExtended extends CrittersBase {
readonly warnings: string[] = [];
readonly errors: string[] = [];
private initialEmbedLinkedStylesheet: EmbedLinkedStylesheetFn;
private addedCspScriptsDocuments = new WeakSet<PartialDocument>();
private documentNonces = new WeakMap<PartialDocument, string | null>();

// Inherited from `Critters`, but not exposed in the typings.
protected declare embedLinkedStylesheet: EmbedLinkedStylesheetFn;

constructor(
private readonly optionsExtended: InlineCriticalCssProcessorOptions &
InlineCriticalCssProcessOptions,
Expand All @@ -119,17 +126,11 @@ class CrittersExtended extends Critters {
reduceInlineStyles: false,
mergeStylesheets: false,
// Note: if `preload` changes to anything other than `media`, the logic in
// `embedLinkedStylesheetOverride` will have to be updated.
// `embedLinkedStylesheet` will have to be updated.
preload: 'media',
noscriptFallback: true,
inlineFonts: true,
});

// We can't use inheritance to override `embedLinkedStylesheet`, because it's not declared in
// the `Critters` .d.ts which means that we can't call the `super` implementation. TS doesn't
// allow for `super` to be cast to a different type.
this.initialEmbedLinkedStylesheet = this.embedLinkedStylesheet;
this.embedLinkedStylesheet = this.embedLinkedStylesheetOverride;
}

public override readFile(path: string): Promise<string> {
Expand All @@ -142,7 +143,10 @@ class CrittersExtended extends Critters {
* Override of the Critters `embedLinkedStylesheet` method
* that makes it work with Angular's CSP APIs.
*/
private embedLinkedStylesheetOverride: EmbedLinkedStylesheetFn = async (link, document) => {
override async embedLinkedStylesheet(
link: PartialHTMLElement,
document: PartialDocument,
): Promise<unknown> {
if (link.getAttribute('media') === 'print' && link.next?.name === 'noscript') {
// Workaround for https://github.com/GoogleChromeLabs/critters/issues/64
// NB: this is only needed for the webpack based builders.
Expand All @@ -154,7 +158,7 @@ class CrittersExtended extends Critters {
}
}

const returnValue = await this.initialEmbedLinkedStylesheet(link, document);
const returnValue = await super.embedLinkedStylesheet(link, document);
const cspNonce = this.findCspNonce(document);

if (cspNonce) {
Expand Down Expand Up @@ -182,7 +186,7 @@ class CrittersExtended extends Critters {
}

return returnValue;
};
}

/**
* Finds the CSP nonce for a specific document.
Expand Down
26 changes: 4 additions & 22 deletions packages/angular/ssr/src/app.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,34 +187,16 @@ export class AngularServerApp {

if (manifest.inlineCriticalCss) {
// Optionally inline critical CSS.
const inlineCriticalCssProcessor = this.getOrCreateInlineCssProcessor();
html = await inlineCriticalCssProcessor.process(html);
}

return new Response(html, responseInit);
}

/**
* Retrieves or creates the inline critical CSS processor.
* If one does not exist, it initializes a new instance.
*
* @returns The inline critical CSS processor instance.
*/
private getOrCreateInlineCssProcessor(): InlineCriticalCssProcessor {
let inlineCriticalCssProcessor = this.inlineCriticalCssProcessor;

if (!inlineCriticalCssProcessor) {
inlineCriticalCssProcessor = new InlineCriticalCssProcessor();
inlineCriticalCssProcessor.readFile = (path: string) => {
this.inlineCriticalCssProcessor ??= new InlineCriticalCssProcessor((path: string) => {
const fileName = path.split('/').pop() ?? path;

return this.assets.getServerAsset(fileName);
};
});

this.inlineCriticalCssProcessor = inlineCriticalCssProcessor;
html = await this.inlineCriticalCssProcessor.process(html);
}

return inlineCriticalCssProcessor;
return new Response(html, responseInit);
}
}

Expand Down
5 changes: 2 additions & 3 deletions packages/angular/ssr/src/common-engine/inline-css-processor.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,15 @@ export class CommonEngineInlineCriticalCssProcessor {
private readonly resourceCache = new Map<string, string>();

async process(html: string, outputPath: string | undefined): Promise<string> {
const critters = new InlineCriticalCssProcessor(outputPath);
critters.readFile = async (path: string): Promise<string> => {
const critters = new InlineCriticalCssProcessor(async (path) => {
let resourceContent = this.resourceCache.get(path);
if (resourceContent === undefined) {
resourceContent = await readFile(path, 'utf-8');
this.resourceCache.set(path, resourceContent);
}

return resourceContent;
};
}, outputPath);

return critters.process(html);
}
Expand Down
99 changes: 53 additions & 46 deletions packages/angular/ssr/src/utils/inline-critical-css.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,36 +19,46 @@ const MEDIA_SET_HANDLER_PATTERN = /^this\.media=["'](.*)["'];?$/;
const CSP_MEDIA_ATTR = 'ngCspMedia';

/**
* Script text used to change the media value of the link tags.
* Script that dynamically updates the `media` attribute of `<link>` tags based on a custom attribute (`CSP_MEDIA_ATTR`).
*
* NOTE:
* We do not use `document.querySelectorAll('link').forEach((s) => s.addEventListener('load', ...)`
* because this does not always fire on Chome.
* because load events are not always triggered reliably on Chrome.
* See: https://github.com/angular/angular-cli/issues/26932 and https://crbug.com/1521256
*
* The script:
* - Ensures the event target is a `<link>` tag with the `CSP_MEDIA_ATTR` attribute.
* - Updates the `media` attribute with the value of `CSP_MEDIA_ATTR` and then removes the attribute.
* - Removes the event listener when all relevant `<link>` tags have been processed.
* - Uses event capturing (the `true` parameter) since load events do not bubble up the DOM.
*/
const LINK_LOAD_SCRIPT_CONTENT = [
'(() => {',
` const CSP_MEDIA_ATTR = '${CSP_MEDIA_ATTR}';`,
' const documentElement = document.documentElement;',
' const listener = (e) => {',
' const target = e.target;',
` if (!target || target.tagName !== 'LINK' || !target.hasAttribute(CSP_MEDIA_ATTR)) {`,
' return;',
' }',

' target.media = target.getAttribute(CSP_MEDIA_ATTR);',
' target.removeAttribute(CSP_MEDIA_ATTR);',

// Remove onload listener when there are no longer styles that need to be loaded.
' if (!document.head.querySelector(`link[${CSP_MEDIA_ATTR}]`)) {',
` documentElement.removeEventListener('load', listener);`,
' }',
' };',

// We use an event with capturing (the true parameter) because load events don't bubble.
` documentElement.addEventListener('load', listener, true);`,
'})();',
].join('\n');
const LINK_LOAD_SCRIPT_CONTENT = `
(() => {
const CSP_MEDIA_ATTR = '${CSP_MEDIA_ATTR}';
const documentElement = document.documentElement;
// Listener for load events on link tags.
const listener = (e) => {
const target = e.target;
if (
!target ||
target.tagName !== 'LINK' ||
!target.hasAttribute(CSP_MEDIA_ATTR)
) {
return;
}
target.media = target.getAttribute(CSP_MEDIA_ATTR);
target.removeAttribute(CSP_MEDIA_ATTR);
if (!document.head.querySelector(\`link[\${CSP_MEDIA_ATTR}]\`)) {
documentElement.removeEventListener('load', listener);
}
};
documentElement.addEventListener('load', listener, true);
})();
`.trim();

/** Partial representation of an `HTMLElement`. */
interface PartialHTMLElement {
Expand All @@ -74,21 +84,21 @@ interface PartialDocument {
querySelector(selector: string): PartialHTMLElement | null;
}

/** Signature of the `Critters.embedLinkedStylesheet` method. */
type EmbedLinkedStylesheetFn = (
link: PartialHTMLElement,
document: PartialDocument,
) => Promise<unknown>;
/* eslint-disable @typescript-eslint/no-unsafe-declaration-merging */
interface CrittersBase {
embedLinkedStylesheet(link: PartialHTMLElement, document: PartialDocument): Promise<unknown>;
}
class CrittersBase extends Critters {}
/* eslint-enable @typescript-eslint/no-unsafe-declaration-merging */

export class InlineCriticalCssProcessor extends Critters {
private initialEmbedLinkedStylesheet: EmbedLinkedStylesheetFn;
export class InlineCriticalCssProcessor extends CrittersBase {
private addedCspScriptsDocuments = new WeakSet<PartialDocument>();
private documentNonces = new WeakMap<PartialDocument, string | null>();

// Inherited from `Critters`, but not exposed in the typings.
protected declare embedLinkedStylesheet: EmbedLinkedStylesheetFn;

constructor(readonly outputPath?: string) {
constructor(
public override readFile: (path: string) => Promise<string>,
readonly outputPath?: string,
) {
super({
logger: {
// eslint-disable-next-line no-console
Expand All @@ -105,24 +115,21 @@ export class InlineCriticalCssProcessor extends Critters {
reduceInlineStyles: false,
mergeStylesheets: false,
// Note: if `preload` changes to anything other than `media`, the logic in
// `embedLinkedStylesheetOverride` will have to be updated.
// `embedLinkedStylesheet` will have to be updated.
preload: 'media',
noscriptFallback: true,
inlineFonts: true,
});

// We can't use inheritance to override `embedLinkedStylesheet`, because it's not declared in
// the `Critters` .d.ts which means that we can't call the `super` implementation. TS doesn't
// allow for `super` to be cast to a different type.
this.initialEmbedLinkedStylesheet = this.embedLinkedStylesheet;
this.embedLinkedStylesheet = this.embedLinkedStylesheetOverride;
}

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

const returnValue = await this.initialEmbedLinkedStylesheet(link, document);
const returnValue = await super.embedLinkedStylesheet(link, document);
const cspNonce = this.findCspNonce(document);

if (cspNonce) {
Expand Down Expand Up @@ -162,7 +169,7 @@ export class InlineCriticalCssProcessor extends Critters {
}

return returnValue;
};
}

/**
* Finds the CSP nonce for a specific document.
Expand Down

0 comments on commit 5d45ecd

Please sign in to comment.