Skip to content

Commit

Permalink
core: resolve redirected script records (#13751)
Browse files Browse the repository at this point in the history
  • Loading branch information
adamraine authored Mar 17, 2022
1 parent 9e97266 commit 9847c11
Show file tree
Hide file tree
Showing 11 changed files with 124 additions and 13 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/devtools.yml
Original file line number Diff line number Diff line change
Expand Up @@ -170,5 +170,5 @@ jobs:
# errors-expired-ssl errors-infinite-loop
# Disabled because Chrome will follow the redirect first, and Lighthouse will only ever see/run the final URL.
# redirects-client-paint-server redirects-multiple-server redirects-single-server redirects-single-client
run: yarn smoke --runner devtools --shard=${{ matrix.smoke-test-shard }}/${{ strategy.job-total }} --retries=2 --invert-match a11y byte-efficiency byte-gzip dbw errors-expired-ssl errors-infinite-loop lantern-idle-callback-short legacy-javascript metrics-tricky-tti metrics-tricky-tti-late-fcp oopif-requests perf-budgets perf-diagnostics-third-party perf-fonts perf-frame-metrics perf-preload perf-trace-elements pwa redirects-client-paint-server redirects-history-push-state redirects-multiple-server redirects-single-server redirects-single-client screenshot seo-passing seo-tap-targets
run: yarn smoke --runner devtools --shard=${{ matrix.smoke-test-shard }}/${{ strategy.job-total }} --retries=2 --invert-match a11y byte-efficiency byte-gzip dbw errors-expired-ssl errors-infinite-loop lantern-idle-callback-short legacy-javascript metrics-tricky-tti metrics-tricky-tti-late-fcp oopif-requests perf-budgets perf-diagnostics-third-party perf-fonts perf-frame-metrics perf-preload perf-trace-elements pwa redirects-client-paint-server redirects-history-push-state redirects-multiple-server redirects-single-server redirects-single-client redirects-scripts screenshot seo-passing seo-tap-targets
working-directory: ${{ github.workspace }}/lighthouse
15 changes: 15 additions & 0 deletions lighthouse-cli/test/fixtures/redirects-scripts.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
</head>
<body>
<h1>I have a script that redirects</h1>
<script src="/simple-script.js?redirect=%2Funused-javascript.js%3Fgzip%3D1"></script>
<script>
Array.prototype.findIndex = function() {};
</script>
</body>
</html>
9 changes: 9 additions & 0 deletions lighthouse-cli/test/fixtures/unused-javascript.js

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions lighthouse-cli/test/smokehouse/core-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import pwaSvgomg from './test-definitions/pwa-svgomg.js';
import redirectsClientPaintServer from './test-definitions/redirects-client-paint-server.js';
import redirectsHistoryPushState from './test-definitions/redirects-history-push-state.js';
import redirectsMultipleServer from './test-definitions/redirects-multiple-server.js';
import redirectScripts from './test-definitions/redirects-scripts.js';
import redirectsSingleClient from './test-definitions/redirects-single-client.js';
import redirectsSingleServer from './test-definitions/redirects-single-server.js';
import redirectsSelf from './test-definitions/redirects-self.js';
Expand Down Expand Up @@ -112,6 +113,7 @@ const smokeTests = [
redirectsClientPaintServer,
redirectsHistoryPushState,
redirectsMultipleServer,
redirectScripts,
redirectsSingleClient,
redirectsSingleServer,
redirectsSelf,
Expand Down
1 change: 1 addition & 0 deletions lighthouse-cli/test/smokehouse/lighthouse-runners/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ async function internalRun(url, tmpPath, configJson, options) {
`-G=${artifactsDirectory}`,
`-A=${artifactsDirectory}`,
'--port=0',
'--quiet',
];

if (useFraggleRock) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/**
* @license Copyright 2020 The Lighthouse Authors. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

/** @type {LH.Config.Json} */
const config = {
extends: 'lighthouse:default',
settings: {
onlyAudits: [
'legacy-javascript',
'unused-javascript',
],
},
};

/**
* @type {Smokehouse.ExpectedRunnerResult}
*/
const expectations = {
lhr: {
runWarnings: [
/The page may not be loading as expected/,
],
requestedUrl: 'http://localhost:10200/online-only.html?redirect=/redirects-scripts.html',
finalUrl: 'http://localhost:10200/redirects-scripts.html',
audits: {
'unused-javascript': {
details: {
items: [
{
// A sourced script that redirects will use the value of the `src` attribute as it's script URL.
// This check ensures that we resolve the redirect and use the final redirect network request to compute savings.
// We can distinguish using totalBytes because the final request is compressed while the redirect request is not.
url: 'http://localhost:10200/simple-script.js?redirect=%2Funused-javascript.js%3Fgzip%3D1',
totalBytes: '285000 +/- 2000',
},
],
},
},
'legacy-javascript': {
details: {
items: [
{
// An inline script that in a document that redirects will take the destination URL as it's script URL.
url: 'http://localhost:10200/redirects-scripts.html',
subItems: {
items: [
{signal: 'Array.prototype.findIndex'},
],
},
},
],
},
},
},
},
};

export default {
id: 'redirects-scripts',
expectations,
config,
};
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@

const ByteEfficiencyAudit = require('./byte-efficiency-audit.js');
const ModuleDuplication = require('../../computed/module-duplication.js');
const NetworkAnalyzer = require('../../lib/dependency-graph/simulator/network-analyzer.js');
const i18n = require('../../lib/i18n/i18n.js');
const {getRequestForScript} = require('../../lib/script-helpers.js');

const UIStrings = {
/** Imperative title of a Lighthouse audit that tells the user to remove duplicate JavaScript from their code. This is displayed in a list of audit titles that Lighthouse generates. */
Expand Down Expand Up @@ -130,7 +130,6 @@ class DuplicatedJavascript extends ByteEfficiencyAudit {
context.options?.ignoreThresholdInBytes || IGNORE_THRESHOLD_IN_BYTES;
const duplication =
await DuplicatedJavascript._getDuplicationGroupedByNodeModules(artifacts, context);
const mainDocumentRecord = NetworkAnalyzer.findOptionalMainDocument(networkRecords);

/** @type {Map<string, number>} */
const transferRatioByUrl = new Map();
Expand Down Expand Up @@ -164,9 +163,7 @@ class DuplicatedJavascript extends ByteEfficiencyAudit {
/** @type {number|undefined} */
let transferRatio = transferRatioByUrl.get(url);
if (transferRatio === undefined) {
const networkRecord = url === artifacts.URL.finalUrl ?
mainDocumentRecord :
networkRecords.find(n => n.url === url);
const networkRecord = getRequestForScript(networkRecords, script);

if (!script || script.length === undefined) {
// This should never happen because we found the wasted bytes from bundles, which required contents in a ScriptElement.
Expand Down
7 changes: 2 additions & 5 deletions lighthouse-core/audits/byte-efficiency/legacy-javascript.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const ByteEfficiencyAudit = require('./byte-efficiency-audit.js');
const JsBundles = require('../../computed/js-bundles.js');
const i18n = require('../../lib/i18n/i18n.js');
const thirdPartyWeb = require('../../lib/third-party-web.js');
const NetworkAnalyzer = require('../../lib/dependency-graph/simulator/network-analyzer.js');
const {getRequestForScript} = require('../../lib/script-helpers.js');

const UIStrings = {
/** Title of a Lighthouse audit that tells the user about legacy polyfills and transforms used on the page. This is displayed in a list of audit titles that Lighthouse generates. */
Expand Down Expand Up @@ -372,11 +372,8 @@ class LegacyJavascript extends ByteEfficiencyAudit {
let transferRatio = transferRatioByUrl.get(url);
if (transferRatio !== undefined) return transferRatio;

const mainDocumentRecord = NetworkAnalyzer.findOptionalMainDocument(networkRecords);
const script = artifacts.Scripts.find(script => script.url === url);
const networkRecord = url === artifacts.URL.finalUrl ?
mainDocumentRecord :
networkRecords.find(n => n.url === script?.url);
const networkRecord = getRequestForScript(networkRecords, script);

if (!script || script.content === null) {
// Can't find content, so just use 1.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
const ByteEfficiencyAudit = require('./byte-efficiency-audit.js');
const i18n = require('../../lib/i18n/i18n.js');
const computeTokenLength = require('../../lib/minification-estimator.js').computeJSTokenLength;
const {getRequestForScript} = require('../../lib/script-helpers.js');

const UIStrings = {
/** Imperative title of a Lighthouse audit that tells the user to minify the page’s JS code to reduce file size. This is displayed in a list of audit titles that Lighthouse generates. */
Expand Down Expand Up @@ -81,7 +82,7 @@ class UnminifiedJavaScript extends ByteEfficiencyAudit {
for (const script of artifacts.Scripts) {
if (!script.content) continue;

const networkRecord = networkRecords.find(record => record.url === script.url);
const networkRecord = getRequestForScript(networkRecords, script);
const displayUrl = script.name === artifacts.URL.finalUrl ?
`inline: ${script.content.substring(0, 40)}...` :
script.url;
Expand Down
3 changes: 2 additions & 1 deletion lighthouse-core/audits/byte-efficiency/unused-javascript.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const ByteEfficiencyAudit = require('./byte-efficiency-audit.js');
const UnusedJavaScriptSummary = require('../../computed/unused-javascript-summary.js');
const JsBundles = require('../../computed/js-bundles.js');
const i18n = require('../../lib/i18n/i18n.js');
const {getRequestForScript} = require('../../lib/script-helpers.js');

const UIStrings = {
/** Imperative title of a Lighthouse audit that tells the user to reduce JavaScript that is never evaluated during page load. This is displayed in a list of audit titles that Lighthouse generates. */
Expand Down Expand Up @@ -90,7 +91,7 @@ class UnusedJavaScript extends ByteEfficiencyAudit {
const script = artifacts.Scripts.find(s => s.scriptId === scriptId);
if (!script) continue; // This should never happen.

const networkRecord = networkRecords.find(record => record.url === script.url);
const networkRecord = getRequestForScript(networkRecords, script);
if (!networkRecord) continue;

const bundle = bundles.find(b => b.script.scriptId === scriptId);
Expand Down
22 changes: 22 additions & 0 deletions lighthouse-core/lib/script-helpers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/**
* @license Copyright 2022 The Lighthouse Authors. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

/**
* @param {LH.Artifacts.NetworkRequest[]} networkRecords
* @param {LH.Artifacts.Script|undefined} script
* @return {LH.Artifacts.NetworkRequest|undefined}
*/
function getRequestForScript(networkRecords, script) {
if (!script) return;
let networkRequest = networkRecords.find(request => request.url === script.url);
while (networkRequest?.redirectDestination) {
networkRequest = networkRequest.redirectDestination;
}
return networkRequest;
}

module.exports = {getRequestForScript};

0 comments on commit 9847c11

Please sign in to comment.