From f8136367d56b0d51ef2c81dfcc6f8b8d924a7e6f Mon Sep 17 00:00:00 2001 From: Jesus David Garcia Gomez Date: Thu, 10 Dec 2020 13:12:26 -0800 Subject: [PATCH] Fix: Change report severity for small files. Fix #4031 --- packages/hint-http-compression/README.md | 8 ++- packages/hint-http-compression/src/hint.ts | 13 +++-- packages/hint-http-compression/src/types.ts | 1 + .../hint-http-compression/tests/_tests.ts | 49 +++++++++++++++++++ .../hint-http-compression/tests/tests-http.ts | 6 +-- 5 files changed, 68 insertions(+), 9 deletions(-) diff --git a/packages/hint-http-compression/README.md b/packages/hint-http-compression/README.md index cd29ae5d60d..e4fef01ef27 100644 --- a/packages/hint-http-compression/README.md +++ b/packages/hint-http-compression/README.md @@ -1008,10 +1008,12 @@ In the [`.hintrc`][hintrc] file: "http-compression": [ "warning", { "resource": { "": , + "threshold": ... }, "html": { "": , + "threshold": ... } }, @@ -1023,7 +1025,8 @@ In the [`.hintrc`][hintrc] file: Where `` can be one of: `brotli`, `gzip`, or `zopfli`. - +And `threshold` is the maximum size (in bytes) to set the +severity of a report to `hint`. E.g. If you want the hint to check if only the page resources are served compressed using Brotli, and not the page itself, you can use the following configuration in the [`.hintrc`][hintrc]: @@ -1035,7 +1038,8 @@ use the following configuration in the [`.hintrc`][hintrc]: "hints": { "http-compression": [ "warning", { "html": { - "brotli": false + "brotli": false, + "threshold": 1024 } }], ... diff --git a/packages/hint-http-compression/src/hint.ts b/packages/hint-http-compression/src/hint.ts index 2cc25792959..146884172b3 100644 --- a/packages/hint-http-compression/src/hint.ts +++ b/packages/hint-http-compression/src/hint.ts @@ -47,6 +47,7 @@ export default class HttpCompressionHint implements IHint { return { brotli: true, gzip: true, + threshold: 1024, zopfli: true, ...(context.hintOptions && context.hintOptions[property]) }; @@ -57,6 +58,10 @@ export default class HttpCompressionHint implements IHint { // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + const isBigFile = (size: number, options: CompressionCheckOptions) => { + return size > options.threshold; + }; + const checkIfBytesMatch = (rawResponse: Buffer, magicNumbers: number[]) => { return rawResponse && magicNumbers.every((b, i) => { return rawResponse[i] === b; @@ -281,7 +286,7 @@ export default class HttpCompressionHint implements IHint { return !checkIfBytesMatch(rawResponse, [0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x03]); }; - const checkBrotli = async (resource: string, element: HTMLElement | null) => { + const checkBrotli = async (resource: string, element: HTMLElement | null, options: CompressionCheckOptions) => { let networkData = await getNetworkData(resource, { 'Accept-Encoding': 'br' }); if (!networkData) { @@ -340,7 +345,7 @@ export default class HttpCompressionHint implements IHint { context.report( resource, getMessage('compressedWithBrotliOverHTTPS', context.language), - { element, severity: Severity.warning } + { element, severity: isBigFile(rawResponse.byteLength, options) ? Severity.warning : Severity.hint } ); return; @@ -427,7 +432,7 @@ export default class HttpCompressionHint implements IHint { context.report( resource, generateGzipCompressionMessage('gzip'), - { element, severity: Severity.error }); + { element, severity: isBigFile(rawResponse.byteLength, shouldCheckIfCompressedWith) ? Severity.error : Severity.hint }); return; } @@ -844,7 +849,7 @@ export default class HttpCompressionHint implements IHint { } if (shouldCheckIfCompressedWith.brotli) { - await checkBrotli(resource, element); + await checkBrotli(resource, element, shouldCheckIfCompressedWith); } }; diff --git a/packages/hint-http-compression/src/types.ts b/packages/hint-http-compression/src/types.ts index fbdebd206b0..9aa62625d36 100644 --- a/packages/hint-http-compression/src/types.ts +++ b/packages/hint-http-compression/src/types.ts @@ -1,5 +1,6 @@ export type CompressionCheckOptions = { brotli: boolean; gzip: boolean; + threshold: number; // bytes zopfli: boolean; }; diff --git a/packages/hint-http-compression/tests/_tests.ts b/packages/hint-http-compression/tests/_tests.ts index 16ccc8949c9..0248dc6ccc5 100644 --- a/packages/hint-http-compression/tests/_tests.ts +++ b/packages/hint-http-compression/tests/_tests.ts @@ -248,6 +248,17 @@ const testsForBrotli: HintTest[] = [ scriptFileHeaders: { 'Content-Encoding': null } }) }, + { + name: `Resource is not served compressed with Brotli when Brotli compression is requested but it is a very small file`, + reports: [{ + message: generateCompressionMessage('Brotli', false, 'over HTTPS'), + severity: Severity.hint + }], + serverConfig: createBrotliServerConfig({ + scriptFileContent: scriptSmallFile.original, + scriptFileHeaders: { 'Content-Encoding': null } + }) + }, { name: `Resource is served compressed with Brotli and without the 'Content-Encoding' header when Brotli compression is requested`, reports: [{ @@ -445,6 +456,29 @@ const testsForDisallowedCompressionMethods = (https: boolean = false): HintTest[ https ) }, + { + name: `Compressed resource is served with disallowed 'Content-Encoding: x-compress' header but it is a very small file`, + reports: [ + { + message: generateDisallowedCompressionMessage('x-compress'), + severity: Severity.warning + }, + { + message: generateCompressionMessage('gzip'), + severity: Severity.hint + } + ], + serverConfig: createGzipZopfliServerConfig( + { + scriptFileContent: scriptSmallFile.original, + scriptFileHeaders: { + 'Content-Encoding': 'x-compress', + vary: 'Accept-Encoding' + } + }, + https + ) + }, { name: `Compressed resource is served with 'Get-Dictionary' header`, reports: [{ @@ -482,6 +516,21 @@ const testsForGzipZopfli = (https: boolean = false): HintTest[] => { https ) }, + { + name: `Resource is not served compressed with gzip when gzip compression is requested but it is a very small file`, + reports: [{ + message: generateCompressionMessage('gzip'), + severity: Severity.hint + }], + serverConfig: createGzipZopfliServerConfig( + { + request: { headers: { 'Accept-Encoding': 'gzip' } }, + scriptFileContent: scriptSmallFile.original, + scriptFileHeaders: { 'Content-Encoding': null } + }, + https + ) + }, { name: `Resource is served compressed with gzip and without the 'Content-Encoding' header when gzip compression is requested`, reports: [ diff --git a/packages/hint-http-compression/tests/tests-http.ts b/packages/hint-http-compression/tests/tests-http.ts index a00d4e645ae..aa1d631637d 100644 --- a/packages/hint-http-compression/tests/tests-http.ts +++ b/packages/hint-http-compression/tests/tests-http.ts @@ -37,14 +37,14 @@ testHint(hintPath, testsForGzipZopfliUASniffing(), testConfigs); testHint(hintPath, testsForBrotliOverHTTP, testConfigs); // Tests for the user options. -[true, false].forEach((isTarget) => { +[true, false].forEach((isHTML) => { ['gzip', 'zopfli', 'brotli'].forEach((encoding) => { testHint( hintPath, - testsForUserConfigs(`${encoding}`, isTarget), + testsForUserConfigs(`${encoding}`, isHTML), { ...testConfigs, - hintOptions: { [isTarget ? 'target' : 'resource']: { [encoding]: false } } + hintOptions: { [isHTML ? 'html' : 'resource']: { [encoding]: false } } } ); });