-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Architecture surface refactor and new node/ top-level entry point #14048
Comments
I like this proposal. The layering feels right . I see a lot of room for improvement in that exported API, so I'm excited to get to that part. |
blockers:
|
now that the above blockers have settled, @brendankenny do you still think this is good for 10.0? Is there a reasonable way to break this down or would it need to happen all in one go? |
@paulirish and I discussed splitting this into the architecture refactor for 10.0, and the filesystem restructure after 10.0. We also constructed an export surface for 10.0, but since paul's computer broke I gotta go off memory: // Default
export default lighthouse;
export {
// 1st tier
navigation,
startTimespan,
snapshot,
startFlow,
generateReport(lhrOrFlow), //? also csv?
// Secondary
legacyNavigation,
auditFlowArtifacts,
getAuditList,
traceCategories,
}
// For plugins
export {Audit} from './audits/audit.js';
export {default as Gatherer} from './gather/base-gatherer.js';
export {NetworkRecords} from './computed/network-records.js';
|
@adamraine We use |
Thanks! I edited your comment to group things a lil bit too.
This seems exposed only for us. I think we shouldnt expose as public api.
Looks like this goes back to #367 Supporting it doesn't seem hard, but i don't think this CLI flag is that important...... ?
This goes back to #453 and #420 which showed the usecase. Is also probably something that should be available for us, but not public.
We exposed this for plugins, too. https://github.com/GoogleChrome/lighthouse/blob/main/docs/plugins.md#using-network-requests
Hmm. It doesn't feel like we do. I think @benschwarz is doing some advanced programmatic use, like this but.. with -G/-A stuff, too? Ben, the 10.0 apis will definitely change things up on you... so let's sync on that. (But seeing as those configJson is supported at the entrypoint of the FR methods.. i suspect generateConfig aint needed in a 10.x world) |
We're headlining Fraggle Rock in the Lighthouse 10.0 release, asking folks to try out a new API, so this is the best opportunity to make substantial changes to the Legacy and Fraggle-Rock-preview APIs for understandability and easy of use.
Our current architecture is something like:
(feel free to imagine a better diagram :)
There's also an implicit client in the top level: node users enter into
core/
directly and that exacerbates the main issue, which is how things are divided up. eg. chrome-launching is incli/
, so is an annoying detail to handle yourself if using the node module, while -GA is incore
, which can cause issues with bundling since the other clients don't have a disk to load from or save to. You also end up with having to anticipate how these layers interact making things more complex and requiring support for core to possibly connect to a browser rather than simply having aPage
passed in or not.Proposal
Instead, we make a place for those decisions while simultaneously making
cli/
andcore/
simpler and the node experience more featureful.I've played with a few refactors, but I'm very partial to @patrickhulce's suggestion in #12393 (comment), which is basically make a new top-level
node/
directory that handles stuff currently incli/
that isn't specific to a CLI, and stuff currently incore/
that (as a general rule of thumb) isn't needed for the DevTools and LR bundles:New
cli/
node/
-GA
main
)Some of
core/
andnode/
would be repeating each other (like a lot of re-exporting what's infraggle-rock/api.js
, wherever that ends up), but it wouldn't be that large and it would allow for a well documented and curated main export from lighthouse, free from e.g. the extras that the different clients need out ofcore/
.API
As a rough starting point (more or less what we have now):
The biggest opportunity for improvement is probably a pass or two rationalizing
configJson
,flags
, andconfigContext
before we foist them on the world :)There's also a lot of legacy re-exports, that I think it would be helpful to revisit. e.g.
lighthouse.Audit
andlighthouse.Gatherer
base-gatherer
has no imports of its own, just types. Maybe we should switch just to type enforcement rather than asking custom gatherers to extend a class that does nothing?Audit
stuff is just types, can it also switch to just an interface and all helper functions move off ofAudit
and into a module of audit utilities?Public types
We're at the point that putting together public types could be a useful exercise for improving Lighthouse's API. Through transitive types we currently expose more or less the entire Lighthouse universe through
core/index.js
, and the best places to cut the type graph seem also to be some of the best places to cut our own dependency graph (e.g.base-gatherer.js
andaudit.js
).If we get serious about plugins again, we could publish a separate types package, something like
@types/lighthouse-plugins
/@types/lighthouse-deep-cuts
or whatever, allowing deep requires into select parts of lighthouse to be typed for use in plugins.The text was updated successfully, but these errors were encountered: