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: adjustments to stack-pack comments and types #8169

Merged
merged 2 commits into from
Apr 12, 2019
Merged

core: adjustments to stack-pack comments and types #8169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d28d14c
core: adjustments to stack-pack comments and types
brendankenny Apr 11, 2019
da14c52
feedback
brendankenny Apr 11, 2019
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
3 changes: 2 additions & 1 deletion build/build-bundle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ async function browserifyFile(entryPath, distPath) {

bundle
// Transform the fs.readFile etc into inline strings.
.transform('brfs', {global: true, parserOpts: {ecmaVersion: 9}})
.transform('brfs', {global: true, parserOpts: {ecmaVersion: 10}})
// Strip everything out of package.json includes except for the version.
.transform('package-json-versionify');

Expand Down Expand Up @@ -121,6 +121,7 @@ function minifyScript(filePath) {
shouldPrintComment: () => false, // Don't include @license or @preserve comments either
plugins: [
'syntax-object-rest-spread',
'syntax-async-generators',
Copy link
Member Author

Choose a reason for hiding this comment

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

this change was needed for supporting for await...of in our client minifying pipeline. It feels like we shouldn't need the plugin now that it's stage 4 and shipping everywhere, but I don't care enough to look into babel more :)

note that this isn't transpiling anything (we only minify), it's just so the babel parser understands the syntax and doesn't throw when seeing this.

(no idea if the brfs change is needed, it's just that 10 is the latest version, so might as well)

],
// sourceMaps: 'both'
};
Expand Down
6 changes: 3 additions & 3 deletions lighthouse-core/audits/dobetterweb/js-libraries.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,12 @@ class JsLibrariesAudit extends Audit {
* @return {LH.Audit.Product}
*/
static audit(artifacts) {
const libDetails = (artifacts.Stacks || [])
const libDetails = artifacts.Stacks
.filter(stack => stack.detector === 'js')
.map(stack => ({
name: stack.name,
version: stack.version || undefined, // null if not detected
npm: stack.npm || undefined, // ~70% of libs come with this field
version: stack.version,
npm: stack.npm,
}));

/** @type {LH.Audit.Details.Table['headings']} */
Expand Down
8 changes: 4 additions & 4 deletions lighthouse-core/audits/dobetterweb/no-vulnerable-libraries.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ class NoVulnerableLibrariesAudit extends Audit {

/**
* Attempts to normalize the version.
* @param {?string} version
* @return {?string}
* @param {string|undefined} version
* @return {string|undefined}
*/
static normalizeVersion(version) {
if (!version) return version;
Expand All @@ -78,7 +78,7 @@ class NoVulnerableLibrariesAudit extends Audit {

/**
* @param {string} normalizedVersion
* @param {{name: string, version: string, npm?: string}} lib
* @param {LH.Artifacts.DetectedStack} lib
* @param {SnykDB} snykDB
* @return {Array<Vulnerability>}
*/
Expand Down Expand Up @@ -135,7 +135,7 @@ class NoVulnerableLibrariesAudit extends Audit {
* @return {LH.Audit.Product}
*/
static audit(artifacts) {
const foundLibraries = (artifacts.Stacks || []).filter(stack => stack.detector === 'js');
const foundLibraries = artifacts.Stacks.filter(stack => stack.detector === 'js');
const snykDB = NoVulnerableLibrariesAudit.snykDB;

if (!foundLibraries.length) {
Expand Down
12 changes: 9 additions & 3 deletions lighthouse-core/gather/gather-runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

const log = require('lighthouse-logger');
const manifestParser = require('../lib/manifest-parser.js');
const stacksGatherer = require('../lib/gatherer-stacks.js');
const stacksGatherer = require('../lib/stack-collector.js');
const LHError = require('../lib/lh-error.js');
const URL = require('../lib/url-shim.js');
const NetworkRecorder = require('../lib/network-recorder.js');
Expand Down Expand Up @@ -412,7 +412,7 @@ class GatherRunner {
NetworkUserAgent: '', // updated later
BenchmarkIndex: 0, // updated later
WebAppManifest: null, // updated later
Stacks: null, // updated later
Stacks: [], // updated later
traces: {},
devtoolsLogs: {},
settings: options.settings,
Expand Down Expand Up @@ -480,12 +480,18 @@ class GatherRunner {
}
await GatherRunner.beforePass(passContext, gathererResults);
await GatherRunner.pass(passContext, gathererResults);

if (isFirstPass) {
// Fetch the manifest, if it exists. Currently must be fetched before gatherers' `afterPass`.
baseArtifacts.WebAppManifest = await GatherRunner.getWebAppManifest(passContext);
baseArtifacts.Stacks = await stacksGatherer(passContext);
}

const passData = await GatherRunner.afterPass(passContext, gathererResults);

if (isFirstPass) {
baseArtifacts.Stacks = await stacksGatherer(passContext);
Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason for moving this after after pass?

Copy link
Member Author

Choose a reason for hiding this comment

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

any reason for moving this after after pass?

so we aren't tracing while it runs. The web manifest one can move out of during the trace too, but it has to be run before gatherer's afterPass, so we'd need a refactor on GatherRunner.afterPass

Copy link
Collaborator

@patrickhulce patrickhulce Apr 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

oh right, maybe add a comment to this effect? it's hard to remember all the steps that happen inside those when glancing at this section :)

}

// Save devtoolsLog, but networkRecords are discarded and not added onto artifacts.
baseArtifacts.devtoolsLogs[passConfig.passName] = passData.devtoolsLog;

Expand Down
68 changes: 37 additions & 31 deletions lighthouse-core/lib/gatherer-stacks.js → lighthouse-core/lib/stack-collector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,45 +15,52 @@

const fs = require('fs');
const libDetectorSource = fs.readFileSync(
require.resolve('js-library-detector/library/libraries.js'),
'utf8'
);
require.resolve('js-library-detector/library/libraries.js'), 'utf8');

/** @typedef {false | {version: string|null}} JSLibraryDetectorTestResult */
/**
* @typedef JSLibraryDetectorTest
* @property {string} icon Essentially an id, useful if no npm name is detected.
* @property {string} url
* @property {string|null} npm npm module name, if applicable to library.
* @property {function(Window): JSLibraryDetectorTestResult | Promise<JSLibraryDetectorTestResult>} test Returns false if library is not present, otherwise returns an object that contains the library version (set to null if the version is not detected).
*/

/**
* @typedef JSLibrary
* @property {string} name
* @property {string} version
* @property {string} npm
* @property {string} iconName
* @property {string} icon
* @property {string|null} version
* @property {string|null} npm
*/

/**
* Obtains a list of detected JS libraries and their versions.
*/
/* istanbul ignore next */
function detectLibraries() {
async function detectLibraries() {
/** @type {JSLibrary[]} */
const libraries = [];

// d41d8cd98f00b204e9800998ecf8427e_ is a consistent prefix used by the detect libraries
// see https://github.com/HTTPArchive/httparchive/issues/77#issuecomment-291320900
/** @type {Record<string, JSLibraryDetectorTest>} */
// @ts-ignore - injected libDetectorSource var
Object.entries(d41d8cd98f00b204e9800998ecf8427e_LibraryDetectorTests).forEach(
async ([name, lib]) => {
// eslint-disable-line max-len
try {
const result = await lib.test(window);
if (result) {
libraries.push({
name: name,
version: result.version,
npm: lib.npm,
iconName: lib.icon,
});
}
} catch (e) {}
}
);
const libraryDetectorTests = d41d8cd98f00b204e9800998ecf8427e_LibraryDetectorTests; // eslint-disable-line

for (const [name, lib] of Object.entries(libraryDetectorTests)) {
try {
const result = await lib.test(window);
if (result) {
libraries.push({
name: name,
icon: lib.icon,
version: result.version,
npm: lib.npm,
});
}
} catch (e) {}
}

return libraries;
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this mean we always returned an empty array before?

seems important enough to warrant a smoketest and separate fix if so

Copy link
Member Author

Choose a reason for hiding this comment

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

does this mean we always returned an empty array before?

when it was `JSLibraries? Yes.

seems important enough to warrant a smoketest and separate fix if so

an artifact smoketest would be excellent (we have an audit smoke test with the js-libraries audit in dbw picking up jquery), but not sure what you mean about fix. This still returns an empty array when nothing is found?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused given your two answers as they seem mutually exclusive :)

Given that we're not Promise.all and .map'ing I'm surprised that the previous code was working at all, so I asked "does this mean we always returned an empty array before?" to which you say "Yes"

Yet our smoketest for this was still passing and you believe there's no fix necessary, so it seems like it wasn't always returning an empty array.

I guess the answer is that because our evaluateAsync Promise wrapping logic in driver waits for all microtasks to complete, it's sort of coincidence that we were still getting all of the synchronously computed ones by .push'ing to the array that was returned after the fact...

Crazy times 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

ohhh, I understand what you were asking. I thought you meant in the case where there were no libraries detected did we always return an array, even if empty :)

Yes, it was buggy but accidentally working except for the workbox test promise, which resolved enough microtasks later (+ whatever else SW infrastructure was doing) that evaluateAsync had already returned the array.

}
Expand All @@ -62,23 +69,22 @@ function detectLibraries() {
* @param {LH.Gatherer.PassContext} passContext
* @return {Promise<LH.Artifacts['Stacks']>}
*/
async function getStacks(passContext) {
async function collectStacks(passContext) {
const expression = `(function () {
${libDetectorSource};
return (${detectLibraries.toString()}());
})()`;

const jsLibraries = /** @type {JSLibrary[]} */ (await passContext.driver.evaluateAsync(
expression
));
/** @type {JSLibrary[]} */
const jsLibraries = await passContext.driver.evaluateAsync(expression);

return jsLibraries.map(lib => ({
detector: /** @type {'js'} */ ('js'),
id: lib.npm || lib.iconName,
id: lib.npm || lib.icon,
name: lib.name,
version: lib.version,
npm: lib.npm,
version: lib.version || undefined,
npm: lib.npm || undefined,
}));
}

module.exports = getStacks;
module.exports = collectStacks;
52 changes: 29 additions & 23 deletions lighthouse-core/lib/stack-packs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,42 +6,48 @@
'use strict';

const stackPacks = require('@lighthouse/stack-packs');
const log = require('lighthouse-logger');

/**
* Pairs consisting of a stack pack's ID and the set of stacks needed to be
* detected in a page to display that pack's advice.
* @type {Array<{packId: string, requiredStacks: Array<string>}>}
*/
const stackPacksToInclude = [{
packId: 'wordpress',
requiredStacks: ['js:wordpress'],
}];

/**
* @param {LH.Artifacts} artifacts
* Returns all packs that match the stacks found in the page.
* @param {LH.Artifacts['Stacks']} pageStacks
* @return {Array<LH.Result.StackPack>}
*/
function getStackPacks(artifacts) {
function getStackPacks(pageStacks) {
/** @type {Array<LH.Result.StackPack>} */
const packs = [];

if (artifacts.Stacks) {
for (const pageStack of artifacts.Stacks) {
const stackPackToIncl = stackPacksToInclude.find(stackPackToIncl =>
stackPackToIncl.requiredStacks.includes(`${pageStack.detector}:${pageStack.id}`));
if (!stackPackToIncl) {
continue;
}

// Grab the full pack definition
const matchedPack = stackPacks.find(pack => pack.id === stackPackToIncl.packId);
if (!matchedPack) {
// we couldn't find a pack that's in our inclusion list, this is weird.
continue;
}

packs.push({
id: matchedPack.id,
title: matchedPack.title,
iconDataURL: matchedPack.iconDataURL,
descriptions: matchedPack.descriptions,
});
for (const pageStack of pageStacks) {
Copy link
Member Author

Choose a reason for hiding this comment

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

this is all just whitespace changes due to removing the if (artifacts.Stacks) { check

const stackPackToIncl = stackPacksToInclude.find(stackPackToIncl =>
stackPackToIncl.requiredStacks.includes(`${pageStack.detector}:${pageStack.id}`));
if (!stackPackToIncl) {
continue;
}

// Grab the full pack definition
const matchedPack = stackPacks.find(pack => pack.id === stackPackToIncl.packId);
if (!matchedPack) {
log.warn('StackPacks',
`'${stackPackToIncl.packId}' stack pack was matched but is not found in stack-packs lib`);
Copy link
Member Author

Choose a reason for hiding this comment

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

except for warning here :)

continue;
}

packs.push({
id: matchedPack.id,
title: matchedPack.title,
iconDataURL: matchedPack.iconDataURL,
descriptions: matchedPack.descriptions,
});
}

return packs;
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ class Runner {
rendererFormattedStrings: i18n.getRendererFormattedStrings(settings.locale),
icuMessagePaths: {},
},
stackPacks: stackPacks.getStackPacks(artifacts),
stackPacks: stackPacks.getStackPacks(artifacts.Stacks),
};

// Replace ICU message references with localized strings; save replaced paths in lhr.
Expand Down
14 changes: 7 additions & 7 deletions lighthouse-core/test/audits/dobetterweb/js-libraries-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,19 @@ describe('Returns detected front-end JavaScript libraries', () => {
const auditResult2 = JsLibrariesAudit.audit({
Stacks: [
{detector: 'js', name: 'lib1', version: '3.10.1', npm: 'lib1'},
{detector: 'js', name: 'lib2', version: null, npm: 'lib2'},
{detector: 'js', name: 'lib2', version: undefined, npm: 'lib2'},
],
});
assert.equal(auditResult2.rawValue, true);

// LOTS of frontend libs
const auditResult3 = JsLibrariesAudit.audit({
Stacks: [
{detector: 'js', name: 'React', version: null, npm: 'react'},
{detector: 'js', name: 'Polymer', version: null, npm: 'polymer-core'},
{detector: 'js', name: 'Preact', version: null, npm: 'preact'},
{detector: 'js', name: 'Angular', version: null, npm: 'angular'},
{detector: 'js', name: 'jQuery', version: null, npm: 'jquery'},
{detector: 'js', name: 'React', version: undefined, npm: 'react'},
{detector: 'js', name: 'Polymer', version: undefined, npm: 'polymer-core'},
{detector: 'js', name: 'Preact', version: undefined, npm: 'preact'},
{detector: 'js', name: 'Angular', version: undefined, npm: 'angular'},
{detector: 'js', name: 'jQuery', version: undefined, npm: 'jquery'},
],
});
assert.equal(auditResult3.rawValue, true);
Expand All @@ -43,7 +43,7 @@ describe('Returns detected front-end JavaScript libraries', () => {
const auditResult = JsLibrariesAudit.audit({
Stacks: [
{detector: 'js', name: 'lib1', version: '3.10.1', npm: 'lib1'},
{detector: 'js', name: 'lib2', version: null, npm: 'lib2'},
{detector: 'js', name: 'lib2', version: undefined, npm: 'lib2'},
],
});
const expected = [
Expand Down
6 changes: 3 additions & 3 deletions lighthouse-core/test/audits/dobetterweb/no-vulnerable-libraries-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const assert = require('assert');
describe('Avoids front-end JavaScript libraries with known vulnerabilities', () => {
describe('#normalizeVersion', () => {
it('should leave valid and unsavable versions untouched', () => {
assert.equal(NoVulnerableLibrariesAudit.normalizeVersion(null), null);
assert.equal(NoVulnerableLibrariesAudit.normalizeVersion(undefined), undefined);
assert.equal(NoVulnerableLibrariesAudit.normalizeVersion('52.1.13'), '52.1.13');
assert.equal(NoVulnerableLibrariesAudit.normalizeVersion('52.1.13-rc.1'), '52.1.13-rc.1');
assert.equal(NoVulnerableLibrariesAudit.normalizeVersion('c0ab71056b936'), 'c0ab71056b936');
Expand All @@ -31,7 +31,7 @@ describe('Avoids front-end JavaScript libraries with known vulnerabilities', ()
Stacks: [
{detector: 'js', name: 'lib1', version: '1.0.0', npm: 'lib1'},
{detector: 'js', name: 'angular', version: '1.1.4', npm: 'angular'},
{detector: 'js', name: 'lib3', version: null, npm: 'lib3'},
{detector: 'js', name: 'lib3', version: undefined, npm: 'lib3'},
],
});
assert.equal(auditResult.rawValue, false);
Expand Down Expand Up @@ -89,7 +89,7 @@ Array [
const auditResult = NoVulnerableLibrariesAudit.audit({
Stacks: [
{detector: 'js', name: 'lib1', version: '3.10.1', npm: 'lib1'},
{detector: 'js', name: 'lib2', version: null, npm: 'lib2'},
{detector: 'js', name: 'lib2', version: undefined, npm: 'lib2'},
],
});
assert.equal(auditResult.rawValue, true);
Expand Down
3 changes: 2 additions & 1 deletion lighthouse-core/test/fixtures/artifacts/empty-artifacts/artifacts.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,6 @@
"URL": {
"requestedUrl": "https://example.com/",
"finalUrl": "https://example.com/"
}
},
"Stacks": []
}
1 change: 1 addition & 0 deletions lighthouse-core/test/fixtures/artifacts/perflog/artifacts.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"requestedUrl": "https://www.reddit.com/r/nba",
"finalUrl": "https://www.reddit.com/r/nba"
},
"Stacks": [],
"MetaElements": [],
"ViewportDimensions": {
"innerWidth": 412,
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/test/gather/gather-runner-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const Config = require('../../config/config');
const unresolvedPerfLog = require('./../fixtures/unresolved-perflog.json');
const NetworkRequest = require('../../lib/network-request.js');

jest.mock('../../lib/gatherer-stacks.js', () => () => Promise.resolve([]));
jest.mock('../../lib/stack-collector.js', () => () => Promise.resolve([]));

class TestGatherer extends Gatherer {
constructor() {
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/test/runner-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const LHError = require('../lib/lh-error.js');

/* eslint-env jest */

jest.mock('../lib/gatherer-stacks.js', () => () => Promise.resolve([]));
jest.mock('../lib/stack-collector.js', () => () => Promise.resolve([]));

describe('Runner', () => {
/** @type {jest.Mock} */
Expand Down
Loading