Skip to content

core(fr): add basic timespan support#11944

Merged
patrickhulce merged 1 commit into
masterfrom
fr_timespan_runner
Jan 13, 2021
Merged

core(fr): add basic timespan support#11944
patrickhulce merged 1 commit into
masterfrom
fr_timespan_runner

Conversation

@patrickhulce

Copy link
Copy Markdown
Collaborator

Summary
Adds basic timespan support to the Fraggle Rock API.

  • Splits api.js into snapshot-runner.js/base-artifacts.js.
  • Adds basic timespan flow to timespan-runner.js.
  • Expands the FRGatherer base class with beforeTimespan/afterTimespan methods.
  • Upgrades ConsoleMessages gatherer to support Fraggle Rock (easiest and highest impact timespan-only gatherer).

Related Issues/PRs
ref #11313
loosely dependent on #11941 (though could be rebased against master if really necessary)

@patrickhulce patrickhulce requested a review from a team as a code owner January 11, 2021 22:03
@patrickhulce patrickhulce requested review from Beytoven and removed request for a team January 11, 2021 22:03
@google-cla google-cla Bot added the cla: yes label Jan 11, 2021
Comment thread lighthouse-core/runner.js
/**
* @param {(runnerData: {requestedUrl: string, config: Config, driverMock?: Driver}) => Promise<LH.Artifacts>} gatherFn
* @param {{config: Config, url?: string, driverMock?: Driver}} runOpts
* @template {LH.Config.Config | LH.Config.FRConfig} TConfig

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is this just some code golf?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I wouldn't be very good at it if it were ;)

this is replacing Config type requirement with TConfig which can be either the legacy or new fraggle rock config, it doesn't care which one so long as they match

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

from how I understand TS they wouldn't have to match, am I missing something that @template does?

(oh, seems you reverted changes to this file, ok)

@patrickhulce patrickhulce Jan 13, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

they're not reverted, just part of the filtering changes #11941 that disappeared after I fixed some merge conflicts :)

from how I understand TS they wouldn't have to match, am I missing something that @template does?

I think so :)

In my understanding @template {Bar} TFoo ~= <TFoo extends Bar>. Once a invocation parameter constrains the type of TConfig to FRConfig, the other parameter must also be constrained because they're the same type.

TS Playground Example

Comment thread lighthouse-core/test/fraggle-rock/api-test-pptr.js
*/
'use strict';

const Driver = require('./gather/driver.js');

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this is just a move, no logic changes here

* @param {LH.Gatherer.GatherMode} mode
* @return {LH.Config.FRConfig['artifacts']}
*/
function filterArtifactsByGatherMode(artifacts, mode) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this is new, but covered by #11941 and not really part of this PR

* @param {LH.Config.FRConfig} config
* @return {LH.BaseArtifacts}
*/
function getBaseArtifacts(config) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this is just moved, nothing special here

*/
'use strict';

const Driver = require('./driver.js');

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this is just moved, no logic changes here

* @param {{page: import('puppeteer').Page, config?: LH.Config.Json}} options
* @return {Promise<{endTimespan(): Promise<LH.RunnerResult|undefined>}>}
*/
async function startTimespan(options) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this is new and the bulk of the logic to review

@patrickhulce patrickhulce changed the base branch from master to filter_snapshot January 12, 2021 01:28

@Beytoven Beytoven left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

Base automatically changed from filter_snapshot to master January 13, 2021 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants