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.

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

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

Merged
merged 3 commits into from
May 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 3 additions & 8 deletions lighthouse-core/audits/legacy-javascript.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,9 @@

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 thirdPartyWeb = require('../lib/third-party-web.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 @@ -338,10 +337,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 +379,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 thirdPartyWeb.isFirstParty(row.url, mainDocumentEntity);
});
return {
score: foundSignalInFirstPartyCode ? 0 : 1,
Expand Down
24 changes: 4 additions & 20 deletions lighthouse-core/audits/third-party-summary.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,10 @@
*/
'use strict';

const thirdPartyWeb = require('third-party-web/httparchive-nostats-subset');

const Audit = require('./audit.js');
const BootupTime = require('./bootup-time.js');
const i18n = require('../lib/i18n/i18n.js');
const thirdPartyWeb = require('../lib/third-party-web.js');
const NetworkRecords = require('../computed/network-records.js');
const MainResource = require('../computed/main-resource.js');
const MainThreadTasks = require('../computed/main-thread-tasks.js');
Expand Down Expand Up @@ -53,21 +52,6 @@ class ThirdPartySummary extends Audit {
};
}

/**
* `third-party-web` throws when the passed in string doesn't appear to have any domain whatsoever.
* We pass in some not-so-url-like things, so make the dependent-code simpler by making this call safe.
* @param {string} url
* @return {ThirdPartyEntity|undefined}
*/
static getEntitySafe(url) {
try {
return thirdPartyWeb.getEntity(url);
} catch (_) {
return undefined;
}
}


/**
*
* @param {Array<LH.Artifacts.NetworkRequest>} networkRecords
Expand All @@ -81,7 +65,7 @@ class ThirdPartySummary extends Audit {
const defaultEntityStat = {mainThreadTime: 0, blockingTime: 0, transferSize: 0};

for (const request of networkRecords) {
const entity = ThirdPartySummary.getEntitySafe(request.url);
const entity = thirdPartyWeb.getEntity(request.url);
if (!entity) continue;

const entityStats = entities.get(entity) || {...defaultEntityStat};
Expand All @@ -93,7 +77,7 @@ class ThirdPartySummary extends Audit {

for (const task of mainThreadTasks) {
const attributeableURL = BootupTime.getAttributableURLForTask(task, jsURLs);
const entity = ThirdPartySummary.getEntitySafe(attributeableURL);
const entity = thirdPartyWeb.getEntity(attributeableURL);
if (!entity) continue;

const entityStats = entities.get(entity) || {...defaultEntityStat};
Expand Down Expand Up @@ -121,7 +105,7 @@ class ThirdPartySummary extends Audit {
const devtoolsLog = artifacts.devtoolsLogs[Audit.DEFAULT_PASS];
const networkRecords = await NetworkRecords.request(devtoolsLog, context);
const mainResource = await MainResource.request({devtoolsLog, URL: artifacts.URL}, context);
const mainEntity = ThirdPartySummary.getEntitySafe(mainResource.url);
const mainEntity = thirdPartyWeb.getEntity(mainResource.url);
const tasks = await MainThreadTasks.request(trace, context);
const multiplier = settings.throttlingMethod === 'simulate' ?
settings.throttling.cpuSlowdownMultiplier : 1;
Expand Down
49 changes: 49 additions & 0 deletions lighthouse-core/lib/third-party-web.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/**
* @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';

const thirdPartyWeb = require('third-party-web/httparchive-nostats-subset');

/** @typedef {import("third-party-web").IEntity} ThirdPartyEntity */

/**
* `third-party-web` throws when the passed in string doesn't appear to have any domain whatsoever.
* We pass in some not-so-url-like things, so make the dependent-code simpler by making this call safe.
* @param {string} url
* @return {ThirdPartyEntity|undefined}
*/
function getEntity(url) {
try {
return thirdPartyWeb.getEntity(url);
} catch (_) {
return undefined;
}
}

/**
* @param {string} url
* @param {ThirdPartyEntity | undefined} mainDocumentEntity
*/
function isThirdParty(url, mainDocumentEntity) {
const entity = getEntity(url);
if (!entity) return false;
if (entity === mainDocumentEntity) return false;
return true;
}

/**
* @param {string} url
* @param {ThirdPartyEntity | undefined} mainDocumentEntity
*/
function isFirstParty(url, mainDocumentEntity) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can rename this? It isn't really a first party check, it's more of a "not a known majorish third party" check. As an example, up in legacy-javascript.js, URL.rootDomainsMatch(row.url, mainResource.url) is being replaced with a thirdPartyWeb.isFirstParty(row.url, mainDocumentEntity);, which is a better choice for that check but also potentially a very different set of results, depending on the test page.

(removing and doing a if (!thirdPartyWeb.isThirdParty()) up above also seems very clear in its intention if we don't want to bikeshed on names)

return !isThirdParty(url, mainDocumentEntity);
}

module.exports = {
getEntity,
isThirdParty,
isFirstParty,
};
14 changes: 13 additions & 1 deletion lighthouse-core/test/audits/legacy-javascript-test.js
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

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
27 changes: 27 additions & 0 deletions lighthouse-core/test/lib/third-party-web-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* @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';

/* eslint-env jest */

const thirdPartyWeb = require('../../lib/third-party-web.js');

describe('third party web', () => {
it('basic', () => {
expect(thirdPartyWeb.isThirdParty('https://www.example.com', undefined)).toBe(false);
expect(thirdPartyWeb.isFirstParty('https://www.example.com', undefined)).toBe(true);
Copy link
Member

@brendankenny brendankenny May 27, 2020

Choose a reason for hiding this comment

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

maybe this is a better example of where the naming gets weird. Just reading this in isolation, I would have no idea why https://www.example.com was determined to be first party :)


expect(thirdPartyWeb.isThirdParty('https://www.googletagmanager.com', undefined)).toBe(true);
expect(thirdPartyWeb.isFirstParty('https://www.googletagmanager.com', undefined)).toBe(false);
});

it('not third party if main document is same entity', () => {
const mainDocumentEntity = thirdPartyWeb.getEntity('https://www.googletagmanager.com');
expect(thirdPartyWeb.isThirdParty('https://www.googletagmanager.com/a.js', mainDocumentEntity)).toBe(false);
expect(thirdPartyWeb.isThirdParty('https://blah.atdmt.com', mainDocumentEntity)).toBe(true);
expect(thirdPartyWeb.isThirdParty('https://www.example.com', mainDocumentEntity)).toBe(false);
});
});