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
Seperate head and body in gatherer? #77
Comments
I'm wondering if we need to change the HTML gatherer to a DOM gatherer. In the Node code we can use JSDOM / cheerio, in the extension use I suspect many of the audits that will want to query DOM will want to do so that way rather than via RegExps. |
sounds good. Will take a look at doing this instead. |
Yeah def give it a go. I do some branching in the manifest gatherer for similar reasons (fetch in extension vs request in Node). This would likely be the same thing. The viewport is the only audit reliant on this atm, so it's impact should be minimal, and the viewport audit has tests so ... 👍 |
BTW if you use |
I've started looking at one way to do this dom like selector in the audits and ended up doing the following - https://github.com/GoogleChrome/lighthouse/blob/html-theme-color-audit/gatherers/html.js I essentially give the audits access to the window OR use JSDom for node environments. (In the chrome extension I have to ignore the jsdom module because babel / browserify doesn't like something in it). @paulirish seems not too happy about JSDom being added, so simple solutions would be:
|
First, on ChromeDriver... It's doing 90% of its browser interaction by via the Chrome Debugging Protocol that we're using directly. So it's pretty much the jQuery to our DOM. And in this case, we're too cool for jQuery. (That and we're unable to do everything we want via ChromeDriver and they dont work side by side) Overall, we should always trust the browser as source of truth and take care when serializing/reparsing over in Node that we maintain 100% fidelity with what the browser sees. I think we have a few options :
Talking with @paullewis and both of us favor 1 or 2, assuming they are done as a specialist gather and definitely not in the audit. They are fairly one-off gathers and rely on a live runtime, but they should be straightforward and less brittle than... 3.. which would be very testable, but would also need ergonomics sugar to handle walking, querySelectoring, etc. @gauntface do you have a preference of 1 or 2? |
I'm probably more inclined for option 2 but curious why it was changed from On Fri, 25 Mar 2016, 00:25 Paul Irish, notifications@github.com wrote:
|
Not sure. Lewis did it a while back. Closing this as it seems to be mostly taken care of with the DOM based approach in the PR now. I'm adding some documentation so that there's justification for this sorta thing. |
Oh, on Chrome Driver. It might make sense to use ChromeDriver for tests, but I have a feeling like it'd break since we require the protocol in either case (although devtools eng is working on fixing that bug). |
note that we already have the latest (continuous build) Chrome downloading and starting up (listening on port 9222) on Travis, so anyone can start writing tests against a live browser today |
At the moment viewport audit is running a regex over the entire html. It would be useful for this (and future audits) to seperate the body and the head as inputs. The other alternative would be to have these as gathers, but that feels like it wouldn't be as flexible.
Thoughts on having input.html + input.head + html body?
The text was updated successfully, but these errors were encountered: