core(fr): initial gatherer migration for snapshot, navigation #11993
Conversation
connorjclark
requested review from
patrickhulce
and removed request for
a team
| @@ -30,10 +30,6 @@ function remoteObjectToString(obj) { | |||
| return `[${type} ${className}]`; | |||
| } | |||
|
|
|||
| /** | |||
| * @implements {LH.Gatherer.GathererInstance} | |||
| * @implements {LH.Gatherer.FRGathererInstance} | |||
There was a problem hiding this comment.
This seemed unnecessary.
There was a problem hiding this comment.
Depends on how explicit we want to be with knowing whether each gatherer supports FR and legacy or not :)
I erred on the side of "it's not yet obvious enough that a require('../../fraggle-rock/gather/base-gatherer.js'); at the top means this gatherer supports both APIs", but if everyone on the team feels it's already clear, wfm 👍
There was a problem hiding this comment.
I would prefer keeping this around to keep it more explicit. The @implements was helpful when reviewing.
There was a problem hiding this comment.
const FRGatherer = require('../../fraggle-rock/gather/base-gatherer.js');
class ... extends FRGatherer
?
There was a problem hiding this comment.
That works for me.
One possible issue is that I already knew FRGatherer supports legacy mode. Probably not a big deal, I was mostly worried about missing the require('../../fraggle-rock/gather/base-gatherer.js'); at the top of the file.
There was a problem hiding this comment.
LGTM, though would love for someone else on the team who I haven't explained anything to if the @implements is helpful for their understanding/clarity.
@adamraine perhaps? :)
| @@ -5,35 +5,40 @@ | |||
| */ | |||
| 'use strict'; | |||
|
|
|||
| const Gatherer = require('./gatherer.js'); | |||
| const Gatherer = require('../../fraggle-rock/gather/base-gatherer.js'); | |||
|
|
|||
| /** | |||
| * @fileoverview Tracks unused CSS rules. | |||
| */ | |||
| class CSSUsage extends Gatherer { | |||
There was a problem hiding this comment.
Conceptually, I don't think this should be a snapshot gatherer. Let's flag it in the FR issue for discussion and a potential rewrite soon?
| const driver = passContext.driver; | ||
|
|
||
| await driver.sendCommand('DOM.enable'); | ||
| await driver.defaultSession.sendCommand('DOM.enable'); |
There was a problem hiding this comment.
hm, unrelated but that's weird why do we enable/disable DOM if everything is in driver.evaluate anyway? 🤔
| const driver = passContext.driver; | ||
|
|
||
| const formElements = await driver.evaluate(collectFormElements, { |
There was a problem hiding this comment.
nit: revert
when I see this structure I assume it's because there was a time where debugging this was necessary and console.log(formElements) seems useful + easy to keep :)
connorjclark
changed the title
connorjclark
changed the title
|
shoot @connorjclark I forgot to ask you to add these to the fraggle rock default config so we can check for any errors, could you pick that up next? |
|
ya |
This migrates the gatherers (14) that only use
afterPass, the interface already supported byFRTransitionalContext, and noloadData.ref #11313