Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

core(legacy-javascript): use third-party-web for scoring #10849

Merged
merged 3 commits into from
May 27, 2020
Merged

core(legacy-javascript): use third-party-web for scoring #10849

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1144971
core(legacy-javascript): use third-party-web for scoring
connorjclark May 27, 2020
ea9d768
type
connorjclark May 27, 2020
bfaab82
pr
connorjclark May 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions lighthouse-core/audits/legacy-javascript.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,10 @@
/** @typedef {{name: string, expression: string}} Pattern */
/** @typedef {{name: string, line: number, column: number}} PatternMatchResult */

const thirdPartyWeb = require('third-party-web/httparchive-nostats-subset');
const Audit = require('./audit.js');
const NetworkRecords = require('../computed/network-records.js');
const MainResource = require('../computed/main-resource.js');
const JSBundles = require('../computed/js-bundles.js');
const URL = require('../lib/url-shim.js');
const i18n = require('../lib/i18n/i18n.js');

const UIStrings = {
Expand All @@ -33,6 +32,21 @@ const UIStrings = {

const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);

/**
* @param {string} url
* @param {import('third-party-web').IEntity | undefined} mainDocumentEntity
*/
function isThirdParty(url, mainDocumentEntity) {
try {
const entity = thirdPartyWeb.getEntity(url);
if (!entity) return false;
if (entity === mainDocumentEntity) return false;
return true;
} catch (_) {
return false;
}
}

/**
* Takes a list of patterns (consisting of a name identifier and a RegExp expression string)
* and returns match results with line / column information for a given code input.
Expand Down Expand Up @@ -338,10 +352,6 @@ class LegacyJavascript extends Audit {
static async audit(artifacts, context) {
const devtoolsLog = artifacts.devtoolsLogs[LegacyJavascript.DEFAULT_PASS];
const networkRecords = await NetworkRecords.request(devtoolsLog, context);
const mainResource = await MainResource.request({
URL: artifacts.URL,
devtoolsLog,
}, context);
const bundles = await JSBundles.request(artifacts, context);

/** @type {Array<{url: string, signals: string[], locations: LH.Audit.Details.SourceLocationValue[]}>} */
Expand Down Expand Up @@ -384,9 +394,9 @@ class LegacyJavascript extends Audit {
const details = Audit.makeTableDetails(headings, tableRows);

// Only fail if first party code has legacy code.
// TODO(cjamcl): Use third-party-web.
const mainDocumentEntity = thirdPartyWeb.getEntity(artifacts.URL.finalUrl);
const foundSignalInFirstPartyCode = tableRows.some(row => {
return URL.rootDomainsMatch(row.url, mainResource.url);
return !isThirdParty(row.url, mainDocumentEntity);
});
return {
score: foundSignalInFirstPartyCode ? 0 : 1,
Expand Down
14 changes: 13 additions & 1 deletion lighthouse-core/test/audits/legacy-javascript-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const createArtifacts = (scripts) => {
url,
}));
return {
URL: {finalUrl: '', requestedUrl: ''},
URL: {finalUrl: 'https://www.example.com', requestedUrl: 'https://www.example.com'},
devtoolsLogs: {defaultPass: networkRecordsToDevtoolsLog(networkRecords)},
ScriptElements: scripts.map(({url, code}, index) => {
return {
Expand Down Expand Up @@ -83,6 +83,18 @@ describe('LegacyJavaScript audit', () => {
assert.equal(result.extendedInfo.signalCount, 0);
});

it('passes code with a legacy polyfill in third party resource', async () => {
const artifacts = createArtifacts([
{
code: 'String.prototype.repeat = function() {}',
url: 'https://www.googletagmanager.com/a.js',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nice to ensure the main entity logic is getting exercised too

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see us re-testing this particular logic over and over again ... maybe just one test in the lib that this fn will move to?

and just assume that usage within audits is OK (treat like a dependency, for example we don't test that the URL class works as expected in every test)

Copy link
Collaborator

@patrickhulce patrickhulce May 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make the mainEntity a required argument, and you've got a deal :) Oh lol it's already required I just had it wrong in my head. Carry on! :)

making it required+typechecking solves exactly my concern, and agreed it would like testing URL at that point.

},
]);
const result = await LegacyJavascript.audit(artifacts, {computedCache: new Map()});
assert.equal(result.score, 1);
assert.equal(result.extendedInfo.signalCount, 1);
});

it('fails code with a legacy polyfill', async () => {
const artifacts = createArtifacts([
{
Expand Down