[go: nahoru, domu]
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(fr): add timespan support to css-usage #12728

Merged
merged 7 commits into from
Jul 19, 2021
Merged

core(fr): add timespan support to css-usage #12728

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d4d10a3
core(fr): add timespan support to css-usage
adamraine Jun 30, 2021
43230fb
comments
adamraine Jul 8, 2021
1c04831
Merge branch 'master' into fr-css-timespan
adamraine Jul 8, 2021
3faf0f4
api
adamraine Jul 8, 2021
c7680ff
comments
adamraine Jul 8, 2021
72adda0
Merge branch 'master' into fr-css-timespan
adamraine Jul 19, 2021
ac25ec1
rm demo
adamraine Jul 19, 2021
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
Next Next commit
core(fr): add timespan support to css-usage
  • Loading branch information
@adamraine
adamraine committed Jun 30, 2021
commit d4d10a3b5c948e90bd4051a817a67b5c5238d3d3
28 changes: 28 additions & 0 deletions demo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
'use strict';
const puppeteer = require('puppeteer');
const lighthouse = require('./lighthouse-core/fraggle-rock/api.js');
const open = require('open');
const fs = require('fs');

const defaultConfig = require('./lighthouse-core/fraggle-rock/config/default-config.js');

async function run() {
const browser = await puppeteer.launch({
headless: false,
});
const page = await browser.newPage();

const config = {...defaultConfig, settings: {output: 'html'}};
const run = await lighthouse.startTimespan({page, config});

await page.goto('https://store.google.com');
await new Promise(r => setTimeout(r, 1000));

const time = await run.endTimespan();
fs.writeFileSync('fr-report-timespan.html', time.report);
open('fr-report-timespan.html');

await page.close();
await browser.close();
}
run();
79 changes: 61 additions & 18 deletions lighthouse-core/gather/gatherers/css-usage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,38 +11,79 @@ const FRGatherer = require('../../fraggle-rock/gather/base-gatherer.js');
* @fileoverview Tracks unused CSS rules.
*/
class CSSUsage extends FRGatherer {
constructor() {
super();
/** @type {Array<LH.Crdp.CSS.StyleSheetAddedEvent>} */
this._stylesheets = [];
/** @param {LH.Crdp.CSS.StyleSheetAddedEvent} sheet */
this._onStylesheetAdded = sheet => this._stylesheets.push(sheet);
/** @type {LH.Crdp.CSS.RuleUsage[]|undefined} */
this._ruleUsage = undefined;
Copy link
Member Author
@adamraine adamraine Jun 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This should always be set by the time we use it, which is why I opted to throw an error on undefined rather than initialize to []. I'm open to changing it though.

}

/** @type {LH.Gatherer.GathererMeta} */
meta = {
// TODO(FR-COMPAT): Add support for timespan.
supportedModes: ['snapshot', 'navigation'],
supportedModes: ['snapshot', 'timespan', 'navigation'],
};

/**
* @param {LH.Gatherer.FRTransitionalContext} context
* @return {Promise<LH.Artifacts['CSSUsage']>}
*/
async getArtifact(context) {
async startCollection(context) {
adamraine marked this conversation as resolved.
Show resolved Hide resolved
const session = context.driver.defaultSession;
const executionContext = context.driver.executionContext;

/** @type {Array<LH.Crdp.CSS.StyleSheetAddedEvent>} */
const stylesheets = [];
/** @param {LH.Crdp.CSS.StyleSheetAddedEvent} sheet */
const => stylesheets.push(sheet);
session.on('CSS.styleSheetAdded', onStylesheetAdded);
session.on('CSS.styleSheetAdded', this._onStylesheetAdded);

await session.sendCommand('DOM.enable');
await session.sendCommand('CSS.enable');
await session.sendCommand('CSS.startRuleUsageTracking');
}

/**
* @param {LH.Gatherer.FRTransitionalContext} context
*/
async stopCollection(context) {
const session = context.driver.defaultSession;
const coverageResponse = await session.sendCommand('CSS.stopRuleUsageTracking');
this._ruleUsage = coverageResponse.ruleUsage;
session.off('CSS.styleSheetAdded', this._onStylesheetAdded);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: swap order of stop/start functions

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you saying both start functions should come before both stop functions?

Copy link
Collaborator
@connorjclark connorjclark Jul 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

not necessarily both, just the ones that are logically paired. it helps with readability when you can move top to bottom and the fns build off each other. so startThing before stopThing is preferred

Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty sure that's the way I had it, but going start/start/stop/stop looks better imo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the counter-point to this (that I happen to agree with) is that the version here intermixes bespoke helper functions with the official gatherer API functions whereas before the "private" methods were grouped away from "public" :)

I don't think either is a big deal, and perhaps this is just down to how familiar the gatherer API methods are to the reader, but I did like the flow of the original. 🤷


/**
* @param {LH.Gatherer.FRTransitionalContext} context
*/
async startSensitiveInstrumentation(context) {
adamraine marked this conversation as resolved.
Show resolved Hide resolved
if (context.gatherMode !== 'timespan') return;
await this.startCollection(context);
}

// Force style to recompute.
// Doesn't appear to be necessary in newer versions of Chrome.
await executionContext.evaluateAsync('getComputedStyle(document.body)');
/**
* @param {LH.Gatherer.FRTransitionalContext} context
*/
async stopSensitiveInstrumentation(context) {
if (context.gatherMode !== 'timespan') return;
await this.stopCollection(context);
}

session.off('CSS.styleSheetAdded', onStylesheetAdded);
/**
* @param {LH.Gatherer.FRTransitionalContext} context
* @return {Promise<LH.Artifacts['CSSUsage']>}
*/
async getArtifact(context) {
const session = context.driver.defaultSession;
const executionContext = context.driver.executionContext;

if (context.gatherMode !== 'timespan') {
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
await this.startCollection(context);

// Force style to recompute.
// Doesn't appear to be necessary in newer versions of Chrome.
await executionContext.evaluateAsync('getComputedStyle(document.body)');

await this.stopCollection(context);
}

// Fetch style sheet content in parallel.
const promises = stylesheets.map(sheet => {
const promises = this._stylesheets.map(sheet => {
const styleSheetId = sheet.header.styleSheetId;
return session.sendCommand('CSS.getStyleSheetText', {styleSheetId}).then(content => {
return {
Expand All @@ -53,15 +94,17 @@ class CSSUsage extends FRGatherer {
});
const styleSheetInfo = await Promise.all(promises);

const ruleUsageResponse = await session.sendCommand('CSS.stopRuleUsageTracking');
await session.sendCommand('CSS.disable');
await session.sendCommand('DOM.disable');

const dedupedStylesheets = new Map(styleSheetInfo.map(sheet => {
return [sheet.content, sheet];
}));

if (!this._ruleUsage) throw new Error('Issue collecting rule usages');

return {
rules: ruleUsageResponse.ruleUsage,
rules: this._ruleUsage,
stylesheets: Array.from(dedupedStylesheets.values()),
};
}
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/test/fraggle-rock/api-test-pptr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ describe('Fraggle Rock API', () => {

const {auditResults, erroredAudits, failedAudits} = getAuditsBreakdown(lhr);
// TODO(FR-COMPAT): This assertion can be removed when full compatibility is reached.
expect(auditResults.length).toMatchInlineSnapshot(`63`);
expect(auditResults.length).toMatchInlineSnapshot(`67`);

expect(erroredAudits).toHaveLength(0);
expect(failedAudits.map(audit => audit.id)).toContain('errors-in-console');
Expand Down