core: convert computed artifact loading to regular require() #6204

brendankenny · 2018-10-10T00:43:32Z

follow up to #5907 and #6151 (and enabled by test changes in #6171, #6195, and #6196).

This PR completes the transition of computed artifacts magically appearing on artifacts (like artifacts.requestNetworkRecords()) and instead being pulled in with plain old require() statements. Our codebase becomes a whole lot easier to follow and reason about after this.

I'll follow up with a pretty minor (non-functional) cleanup PR (and @paulirish wanted to discuss renaming them or at least moving them out of gather/ at that point).

Apologies for the giant PR. Most of this is mechanical; the major architecture/conceptual changes occurred in those earlier PRs. Due to the dependencies between the computed artifacts and the fact that they have to pass a context to each other, once one of these files was updated it turns out they all had to be :) I'm not sure we could have unwound this stack if we had waited much longer to do this refactor.

I've split up the PR into commits grouped by types of change to make the reviewing a little easier on the brain.

7522a60 - update the computed artifact files. Primarily makes all methods static async and have their calls to each other be explicit instead of through computedArtifacts.request*()
82fc891 - update the computedArtifact tests. Mostly adding context to calls for the cache.
0c9e393 - updates to runner, gather-runner, and some build stuff. Mostly deleting computedArtifacts initialization and custom-bundling code :)
472bef3 - audit updates. Such minimal changes!
ed3c3c7 - updates to audit tests. Mostly adding context to calls.

Or just look at the whole thing :)

brendankenny · 2018-10-10T00:51:01Z

lighthouse-core/gather/computed/new-computed-artifact.js

+ * 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
+ * @param {C} computableArtifact


it was a nice dream to encapsulate computed artifacts like they were before this PR, leaving only a

{name: string, request: (artifacts: Whatever, context: LH.Audit.Context): Promise<Yeah>}

but we access static methods on the computed artifact classes in a few different places and LanternFirstCPUIdle even extends LanternInteractive, so we really need this function to return a

InputComputedArtifact & {request: (artifacts: Whatever, context: LH.Audit.Context): Promise<Yeah>}

which this change does. There's some limitations with current tsc (3.1) and trying to do this in jsdoc (e.g. you can constrain templates in jsdoc but you can't provide default values), but being more explicit with some of these annotations (as the comment added below mentions) makes it work without being overly prescriptive or falling back to any.

I suppose we could dream 😄

later I'd be on board for returning these to nice clean objects and moving those metric helpers into helpers functions instead of OO spaghetti 👍

patrickhulce

wowza

NICE. LEE. DONE.

🔧 🛠 💥 💯

now quick before a conflict! :D

patrickhulce · 2018-10-10T01:57:20Z

lighthouse-core/gather/computed/computed-artifact.js

@@ -9,7 +9,7 @@ const ArbitraryEqualityMap = require('../../lib/arbitrary-equality-map');

 class ComputedArtifact {
  /**
-   * @param {LH.ComputedArtifacts} allComputedArtifacts
+   * @param {*} allComputedArtifacts


what's the longer term plan for new-computed-artifact and this file? or is new-computed-artifact meant to be like the verb constructor not the new file and I've misread it the whole time... 😮

what's the longer term plan for new-computed-artifact and this file?

that's the cleanup followup I mentioned. I'll delete that one, rename the new- file, then I wanted to add some tests and possibly do the file move.

patrickhulce · 2018-10-10T01:59:00Z

lighthouse-core/audits/metrics/speed-index.js

@@ -7,6 +7,7 @@

 const Audit = require('../audit');
 const i18n = require('../../lib/i18n/i18n.js');
+const ComputedSi = require('../../gather/computed/metrics/speed-index.js');


patrickhulce · 2018-10-10T01:59:19Z

lighthouse-core/audits/metrics/first-cpu-idle.js

@@ -7,6 +7,7 @@

 const Audit = require('../audit');
 const i18n = require('../../lib/i18n/i18n.js');
+const ComputedFci = require('../../gather/computed/metrics/first-cpu-idle.js');


ComputedFci 🤢

I couldn't think of another name since SpeedIndex is already taken by the audit class itself and ComputedFirstCPUIdle seemed way too long. I spent exactly 0 time coming up with the name, though, so I'm happy to change :)

ComputedFci 🤢

Unless you mean the capitalization scheme, in which case I WILL DIE ON THIS HILL

Unless you mean the capitalization scheme

Hahaha, that is exactly what I mean :)

Then shouldn't below be FirstCpuIdle? :)

ha, @paulirish caught the same thing :) My defense is that I was merely moving the existing line in the file, git diff just can't handle visualizing it :P

(also that--even though it totally is the way better way--we gave up on consistency on this particular issue a long time ago because no one manages to remember to enforce their favorite way every time. See also: XMLHttpRequest)

patrickhulce · 2018-10-10T01:59:46Z

lighthouse-core/audits/metrics/estimated-input-latency.js

@@ -7,6 +7,7 @@

 const Audit = require('../audit');
 const i18n = require('../../lib/i18n/i18n.js');
+const ComputedEil = require('../../gather/computed/metrics/estimated-input-latency.js');


patrickhulce · 2018-10-10T02:01:16Z

lighthouse-core/gather/computed/metrics/lantern-first-cpu-idle.js

@@ -56,8 +52,19 @@ class LanternFirstCPUIdle extends LanternInteractive {
      longTasks.push({start: timing.startTime, end: timing.endTime});
    }

+    // Require here to resolve circular dependency.
+    const FirstCPUIdle = require('./first-cpu-idle');


this scares me a little, why does that require this?

I found the new structure for computeSimulatedMetrics 👍

I found the new structure for computeSimulatedMetrics 👍

yeah, this seemed fine for now since it's just two places, but we can always refactor into helper functions for things like findQuietWindow

patrickhulce · 2018-10-10T02:04:42Z

lighthouse-core/gather/computed/new-computed-artifact.js

+ * 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
+ * @param {C} computableArtifact


I suppose we could dream 😄

later I'd be on board for returning these to nice clean objects and moving those metric helpers into helpers functions instead of OO spaghetti 👍

brendankenny · 2018-10-12T22:26:19Z

Breaking everybody, sry :)

brendankenny added 5 commits October 9, 2018 17:18

core: convert computed artifact loading to regular require()

7522a60

update computed artifact tests

82fc891

update core/build files

0c9e393

update audits

472bef3

update audit tests

ed3c3c7

brendankenny requested review from patrickhulce and paulirish as code owners October 10, 2018 00:43

brendankenny commented Oct 10, 2018

View reviewed changes

test-lantern fix, dangers of ts-ignore :)

b0c2461

patrickhulce approved these changes Oct 10, 2018

View reviewed changes

Merge branch 'master' into byerequested

aba4198

paulirish approved these changes Oct 12, 2018

View reviewed changes

brendankenny merged commit 3b8ae35 into master Oct 12, 2018

brendankenny deleted the byerequested branch October 12, 2018 22:26

This was referenced Nov 20, 2018

misc: rename core/gather/computed/ to core/computed/ #6618

Merged

core(computed-artifacts): fix cache and add perf timing #6619

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: convert computed artifact loading to regular require() #6204

core: convert computed artifact loading to regular require() #6204

brendankenny commented Oct 10, 2018 •

edited by paulirish

Loading

brendankenny Oct 10, 2018 •

edited

Loading

patrickhulce Oct 10, 2018

patrickhulce left a comment

patrickhulce Oct 10, 2018

brendankenny Oct 10, 2018

patrickhulce Oct 10, 2018

patrickhulce Oct 10, 2018

brendankenny Oct 10, 2018

brendankenny Oct 10, 2018

patrickhulce Oct 10, 2018

connorjclark Oct 13, 2018

brendankenny Oct 13, 2018

brendankenny Oct 13, 2018

patrickhulce Oct 10, 2018

patrickhulce Oct 10, 2018

patrickhulce Oct 10, 2018

brendankenny Oct 10, 2018

patrickhulce Oct 10, 2018

patrickhulce Oct 10, 2018

brendankenny commented Oct 12, 2018

core: convert computed artifact loading to regular require() #6204

core: convert computed artifact loading to regular require() #6204

Conversation

brendankenny commented Oct 10, 2018 • edited by paulirish Loading

brendankenny Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Oct 12, 2018

brendankenny commented Oct 10, 2018 •

edited by paulirish

Loading

brendankenny Oct 10, 2018 •

edited

Loading