core(computed-artifact): remove settings and options from context #12435
Conversation
| @@ -11,7 +11,7 @@ const log = require('lighthouse-logger'); | |||
| /** | |||
| * Decorate computableArtifact with a caching `request()` method which will | |||
| * automatically call `computableArtifact.compute_()` under the hood. | |||
| * @template {{name: string, compute_(artifacts: unknown, context: LH.Audit.Context): Promise<unknown>}} C | |||
| * @template {{name: string, compute_(artifacts: unknown, context: LH.Gatherer.ComputedContext): Promise<unknown>}} C | |||
There was a problem hiding this comment.
I don't know that it belongs in either Audit or Gatherer, maybe in Artifacts namespace?
There was a problem hiding this comment.
Yeah Artifacts sounds better.
| * @param {{URL: LH.Artifacts['URL'], devtoolsLog: LH.DevtoolsLog}} data | ||
| * @param {LH.Audit.Context} context | ||
| * @param {{URL: LH.Artifacts['URL'], devtoolsLog: LH.DevtoolsLog, settings: ImmutableObject<LH.Config.Settings>}} data | ||
| * @param {LH.Gatherer.ComputedContext} context | ||
| * @return {Promise<Record<LH.Budget.ResourceType,ResourceEntry>>} | ||
| */ | ||
| static async compute_(data, context) { |
There was a problem hiding this comment.
do you think this should just accept a budget at this point rather than all of settings?
There was a problem hiding this comment.
I'm going to make this change.
Only problem I found was in the test file for computed/resource-summary.js. The tests expect the first arg to be a subset of artifacts. Making this change disrupts that naming convention.
|
from @@ -6251,6 +6251,12 @@
"duration": 100,
"entryType": "measure"
},
+ {
+ "startTime": 0,
+ "name": "lh:computed:MainResource",
+ "duration": 100,
+ "entryType": "measure"
+ },
{
"startTime": 0,
"name": "lh:audit:timing-budget",
❌ FAIL. LHR has changed.a good example where you think you're getting a cached computed artifact but you're not :) In this case |
| @@ -189,6 +190,10 @@ declare global { | |||
| } | |||
|
|
|||
| module Artifacts { | |||
| export type ComputedContext = Immutable<{ | |||
There was a problem hiding this comment.
Kind of a tangent for where this should live now, this is the kind of thing I think is a good argument for us moving away from global types (no one needs to reference this type except computed artifacts, which already need to import ComputedArtifact, so that's a good namespace for the type to live in), but typescript really needs to fix microsoft/TypeScript#41825 before that's a very ergonomic approach for us in general.
Yeah, this was hard to find. There may be more instances of this that we don't cover in our tests because the artifacts are only requested once. |
Splitting this out from #12427 because it touches a lot of files.
Previously, calculating a computed artifact in the gathering stage required us to add
settingsandoptionsto the context to be compatible withLH.Audit.Context. This PR adds a new context with justcomputedCachethat computed artifacts use.Right now the new context is
LH.Gatherer.ComputedContext. Changing that should be a pretty simple find and replace so I'm open to other suggestions on where it should go.