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

core(fr): align base artifacts with legacy gather-runner #12510

Merged
merged 7 commits into from
Jun 2, 2021

Conversation

patrickhulce
Copy link
Collaborator

Summary
Brings the base artifacts support in FR up to snuff with legacy gather runner.

Related Issues/PRs
ref #11313

Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

Will base artifacts be available in gatherers as dependencies, or can we just pass them in FRTransitionalContext separately?

function finalizeArtifacts(baseArtifacts, gathererArtifacts) {
const warnings = baseArtifacts.LighthouseRunWarnings
.concat(gathererArtifacts.LighthouseRunWarnings || []);
warnings.push(...getEnvironmentWarnings({
Copy link
Member

Choose a reason for hiding this comment

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

nit: use another concat here?

// TODO(FR-COMPAT): This assertion can be removed when full compatibility is reached.
expect(auditResults.length).toMatchInlineSnapshot(`112`);
// TODO(FR-COMPAT): This assertion can be removed when full compatibility is reached. HEAD
expect(auditResults.length).toMatchInlineSnapshot(`141`);
Copy link
Member

Choose a reason for hiding this comment

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

👀

@patrickhulce
Copy link
Collaborator Author

Will base artifacts be available in gatherers as dependencies, or can we just pass them in FRTransitionalContext separately?

Great question, this does not yet enable base artifacts as dependencies. We can talk about how to accomplish that when the need arises (has it already come up for you?). URL is the biggest one I can think of, but that's already available on the phaseContext.

@adamraine
Copy link
Member

We can talk about how to accomplish that when the need arises (has it already come up for you?)

Yes, I know script-elements uses HostFormFactor.

I think it would be easier to include base artifacts separately in the context, or include them as dependencies for all gatherers by default. Essentially I want to avoid writing:

/** @type {LH.Gatherer.FRTransitionalContext<'HostFormFactor'>} */
meta = {
	supportedModes = ['navigation'],
	dependencies: {HostFormFactor: HostFormFactor.symbol},
}

@patrickhulce
Copy link
Collaborator Author

I think it would be easier to include base artifacts separately in the context

I was really hoping to avoid this for the reason that it's not obvious when base artifacts are actually populated and if they're available at the time they would be needed for the dependency. Making artifact dependency relationships explicit is pretty nice FR result.

Essentially I want to avoid writing:

That would actually be my ideal approach 😄 HostFormFactor and HostUserAgent actually make a lot of sense as regular artifacts too... I think what we need is the concept of artifacts that are always collected by default but aren't "special", to solve this.

Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

For our current set of Gatherers, it looks like we only need to depend on HostFormFactor and Settings. Explicit dependencies probably make sense here since there are so few instances of us depending on base artifacts.

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.

4 participants