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.

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

docs: add link to gist for using lighthouse audits directly #10480

Merged
merged 4 commits into from
Mar 30, 2020

Conversation

connorjclark
Copy link
Collaborator

I've had to do similar things a few times for personal tools I've written leveraging Lighthouse. There's actually a legit use case now (internal, idk public so I won't say more) so here's a recipe showing how.

Surely there would be more needed to run other audits directly, however I focused on the current use case at hand. Could add more examples if someone expresses interest.

@connorjclark connorjclark requested a review from a team as a code owner March 18, 2020 22:13
@connorjclark connorjclark requested review from patrickhulce and removed request for a team March 18, 2020 22:13
@connorjclark connorjclark requested review from paulirish and removed request for patrickhulce March 18, 2020 22:13
@connorjclark
Copy link
Collaborator Author

connorjclark commented Mar 18, 2020

(I overrode the auto-assignment b.c. this is something me and paul are working on. but feel free to comment @patrickhulce :)


const fs = require('fs');

const makeDevtoolsLog = require('lighthouse/lighthouse-core/test/network-records-to-devtools-log.js');
Copy link
Member

Choose a reason for hiding this comment

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

warning: this was written to create devtools logs good enough for tests, not necessarily good in and of themselves :)

The intent was to keep improving the generation but I think there's only been a need for that once.

};
}

async function run() {
Copy link
Member

Choose a reason for hiding this comment

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

let's allow @prateekbh to use this file as a lib.

i'm thinking we do a

module.exports = function analyze(arrayOfBundleAndMapCombos){ ...

so basically move L55-60 and L78 out into an index.js file or whatever.

Copy link
Collaborator Author

@connorjclark connorjclark Mar 19, 2020

Choose a reason for hiding this comment

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

We discussed offline, but we do not want to have this file be a module that is used by a client. We don't want to support such an interface. This is just an example recipe. Also it's too early to know what the requirements will be exactly :)

Copy link
Member

Choose a reason for hiding this comment

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

We discussed offline, but we do not want to have this file be a module that is used by a client. We don't want to support such an interface. This is just an example recipe. Also it's too early to know what the requirements will be exactly :)

yeah, that makes sense, but maybe still consider leveraging auditMode in the recipe since absorbing the complexities of running audits is pretty much its purpose.

I don't know how gross it'll be to use this way, but possibly less than dealing with audit products directly :)

Copy link
Member

@paulirish paulirish Mar 25, 2020

Choose a reason for hiding this comment

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

@prateekbh connor convinced me its fine to keep this recipe super hardcoded because anyone we expect to use it is gonna have to do some work. :).

but basically feel free to take the file and do the changes i suggested on your own.

if you continue using this and it makes sense for us to offer this as a real supported API, we can do that (eg. lighthouse.analyzedJSBundles(arr)), but it's too early right now to commit to that.

and please chat with us as you play around with this. :)

Choose a reason for hiding this comment

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

Sounds good to me. Will get in touch

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

total bikeshedding, but is there a different name for this? It's not really (just) using lighthouse modules directly (we've had example code for that for ages), it's more about BYO-artifacts


// Run a byte-efficiency audit directly.
const duplicatedJavascriptResult =
await DuplicatedJavascript.audit_(artifacts, networkRecords, context);
Copy link
Member

Choose a reason for hiding this comment

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

don't you still want to call byte-efficiency audits with .audit()? Most people probably wouldn't want the intermediate result.

Copy link
Member

Choose a reason for hiding this comment

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

don't you still want to call byte-efficiency audits with .audit()? Most people probably wouldn't want the intermediate result.

speaking of which, we do have the slightly awkward thing that audits return an LH.Audit.Product, then it's Runner that calls Audit.generateAuditResult() to turn that into an LH.Audit.Result (which is what you might expect going off of LHR types). @paulirish's idea of a lib for this might be able to hide that away, though (it could really just generate the artifacts and call -A mode).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

don't you still want to call byte-efficiency audits with .audit()? Most people probably wouldn't want the intermediate result.

Don't know yet - maybe not. Use case is still nebulous. I deferred it for now because ByteEfficienyAudit.audit requires a trace.

@paulirish
Copy link
Member

t's not really (just) using lighthouse modules directly (we've had example code for that for ages), it's more about

i was originally thinking audit-as-lib though it's not perfectly suited.

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

we can improve as necessary. seems fine to start with it rough.

};
}

async function run() {
Copy link
Member

@paulirish paulirish Mar 25, 2020

Choose a reason for hiding this comment

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

@prateekbh connor convinced me its fine to keep this recipe super hardcoded because anyone we expect to use it is gonna have to do some work. :).

but basically feel free to take the file and do the changes i suggested on your own.

if you continue using this and it makes sense for us to offer this as a real supported API, we can do that (eg. lighthouse.analyzedJSBundles(arr)), but it's too early right now to commit to that.

and please chat with us as you play around with this. :)

@brendankenny
Copy link
Member

brendankenny commented Mar 25, 2020

we can improve as necessary. seems fine to start with it rough.

Can I interest you in a gist, then? Or an entry in https://github.com/GoogleChrome/lighthouse/blob/master/docs/hacking-tips.md? Or a stackoverflow self-answered post? :)

This is still really rough, will break if the artifacts needed for those still-being-written audits change, and if I'm reading the room correctly no one wants to own building this into a real example (at least until we have user feedback). It doesn't fit well with the other recipes fragility- or quality-wise.

@paulirish
Copy link
Member

we can improve as necessary. seems fine to start with it rough.

Can I interest you in a gist, then? Or an entry in https://github.com/GoogleChrome/lighthouse/blob/master/docs/hacking-tips.md ? :)

okay yeah. sounds good. a gist that's linked from hacking-tips!

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

as discussed gist seems a better home for now

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

groovy. sorry for the shifting goalposts.

@connorjclark connorjclark changed the title docs: add recipe for using lighthouse modules directly docs: add link to gist for using lighthouse audits directly Mar 27, 2020
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.

6 participants