amp-toolbox icon indicating copy to clipboard operation
amp-toolbox copied to clipboard

Lighthouse plugin feedback

Open benschwarz opened this issue 4 years ago • 6 comments

Hi @alabiaga and team 👋

I've some feedback on the Lighthouse plugin that was authored last week. I wasn't sure where else to share it but if there's a better way to communicate please let me know.

At the moment the plugin uses fetch to retrieve the HTML of the requested page. The problem with this approach is that in many cases Lighthouse users have prequalified logins, are setting extra http headers, cookies or other general setup items that ensure that the correct environment is being tested.

In order for this plugin to be used by a wide audience, it'll need to retrieve the HTML body of the main page from Lighthouse itself. As far as I'm aware, there isn't a Gatherer to do this but I'm sure that the html-without-javascript gatherer could be used as a starting point.

As far as I know custom gatherers are not able to be implemented by a plugin. This would need to be added to lighthouse-core directly (seems like a good addition to me!). Maybe someone from @GoogleChrome/lighthouse-hackers can weigh in on that?

benschwarz avatar Sep 24 '19 04:09 benschwarz

@benschwarz Thanks for your input. I actually initially created the AMP validator audit via a custom-config and using a gatherer to pass the html body for validation as I wasn't familiar with the API and didn't think that a plugin would suffice.

Having a gatherer pass the body actually makes sense, as we want to leverage the fact that Lighthouse already allows for this in its lifecycle and using a fetch is an unnecessary call. Though I am trying to better understand the issue with fetch in relation to other Lighthouse users who do the things you mentioned. Are you stating that there are setup items that a custom lighthouse audit does that are blown away by this audit because of the fetch? I don't have deep knowledge of Lighthouse so apologies in advance. It will be easy enough to refactor the code to use gatherers but I am trying to better understand the issue. Thanks

alabiaga avatar Sep 24 '19 08:09 alabiaga

+1 to using the built in gatherers.

@ithinkihaveacat are there other Lighthouse APIs we'd need for an amp-lint integration?

sebastianbenz avatar Sep 24 '19 17:09 sebastianbenz

@alabiaga I checked in with the Lighthouse team to make sure I wasn't giving you phoney information. Sorry about the delay in getting back to you!

Though I am trying to better understand the issue with fetch in relation to other Lighthouse users who do the things you mentioned. Are you stating that there are setup items that a custom lighthouse audit does that are blown away by this audit because of the fetch?

Yep, that's right. The fetch call won't make the request with the same settings and could easily yield results like "theres no amp on this page" because it won't have: The same user-agent, cookies, headers, localstorage or other browser-related scenarios that the user has setup.

It will be easy enough to refactor the code to use gatherers

Yeah. Ideally it wouldn't be too hard to do this, but turns out that Lighthouse needs to make some improvements. Here's why:

  • Plugins are not able to provide their own gatherers. (GoogleChrome/lighthouse#7882)
  • There isn't an existing gatherer to return the HTML only… but there probably should be. (GoogleChrome/lighthouse#9756)

benschwarz avatar Sep 30 '19 00:09 benschwarz

@alabiaga In case you didn't spot it on the Lighthouse repo, MainDocumentContent has been merged to master.

benschwarz avatar Oct 16 '19 23:10 benschwarz

@benschwarz Thanks. @sebastianbenz can prioritize the work as we discussed handing this off. I can help if need be. Thanks again.

alabiaga avatar Oct 17 '19 00:10 alabiaga

Nice @alabiaga, let me know if there's anything I can do to help.

benschwarz avatar Nov 05 '19 22:11 benschwarz