-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core(fr): add artifact dependency support #12045
Conversation
* @param {Record<string, LH.Gatherer.PhaseResult>} artifactsById | ||
* @return {Promise<LH.Gatherer.FRTransitionalContext<LH.Gatherer.DependencyKey>['dependencies']>} | ||
*/ | ||
async function collectArtifactDependencies(artifact, artifactsById) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to be async?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you prefer to convert the final await
to a .then
?
or asking why we want to convert the artifact dependencies from promises to the resolved artifacts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or asking why we want to convert the artifact dependencies from promises to the resolved artifacts?
This. I don't really have a suggestion, just trying to understand this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to pass the artifact directly to a gatherer that is depending on it so the burden doesn't fall on consumers to await and handle any errors. Resolving any errors ahead of time also means every individual gatherer doesn't have to remap their dependencies' errors. We do a similar thing for audits today, pass them artifacts, not promises of artifacts and remap their artifacts when they fail.
lighthouse/lighthouse-core/runner.js
Lines 323 to 339 in b292ae0
// If artifact was an error, output error result on behalf of audit. | |
if (artifacts[artifactName] instanceof Error) { | |
/** @type {Error} */ | |
// @ts-expect-error An artifact *could* be an Error, but caught here, so ignore elsewhere. | |
const artifactError = artifacts[artifactName]; | |
Sentry.captureException(artifactError, { | |
tags: {gatherer: artifactName}, | |
level: 'error', | |
}); | |
log.warn('Runner', `${artifactName} gatherer, required by audit ${audit.meta.id},` + | |
` encountered an error: ${artifactError.message}`); | |
// Create a friendlier display error and mark it as expected to avoid duplicates in Sentry | |
const error = new LHError(LHError.errors.ERRORED_REQUIRED_ARTIFACT, | |
{artifactName, errorMessage: artifactError.message}); |
lighthouse-core/test/fraggle-rock/gather/timespan-runner-test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the review! :)
* @param {Record<string, LH.Gatherer.PhaseResult>} artifactsById | ||
* @return {Promise<LH.Gatherer.FRTransitionalContext<LH.Gatherer.DependencyKey>['dependencies']>} | ||
*/ | ||
async function collectArtifactDependencies(artifact, artifactsById) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you prefer to convert the final await
to a .then
?
or asking why we want to convert the artifact dependencies from promises to the resolved artifacts?
Summary
Adds config and runner support for artifact dependencies, see #12017 for expected usage examples and discussion. I know this is a monster PR, but it truly is the minimal set of artifact dependency support that is validated, typechecked, and tested (there isn't even a single gatherer that's upgraded for dependency support in here! 🤯). I do think it's rather helpful to see it all come together rather than jump between different related PRs by potential different reviewers, but we do have few options if we want to reduce the line count.
Related Issues/PRs
ref #11313
ref #12017