core(tsc): add type checking to remaining top-level audits #5090

brendankenny · 2018-05-02T06:25:02Z

critical-request-chains, user-timings, and a switch on the input of the requestLantern*() methods to (I think) what was intended.

With the other two PRs, this covers all the top-level audits except preload, which has major changes in the works in #5062

brendankenny · 2018-05-02T06:28:01Z

lighthouse-cli/test/smokehouse/offline-local/offline-expectations.js

@@ -65,7 +65,7 @@ module.exports = [
      },
      'user-timings': {
        score: 1,
-        displayValue: '',
+        displayValue: '0',


this is just a result of audit.js stringifying displayValue by checking falsyness

lighthouse/lighthouse-core/audits/audit.js

Line 169 in 8fcbb84

const displayValue = result.displayValue ? `${result.displayValue}` : '';

so 0 was getting converted to '' before explicit stringifying in user-timings.js below

brendankenny · 2018-05-02T06:29:09Z

lighthouse-core/audits/uses-rel-preload.js

@@ -3,7 +3,7 @@
 * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
 * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
 */
-
+// @ts-nocheck


I don't believe the changes here will conflict with #5062

brendankenny · 2018-05-02T06:29:58Z

lighthouse-core/test/audits/is-on-https-test.js

-      {url: 'http://insecure.com/image.jpeg', scheme: 'http', domain: 'insecure.com'}, // should be de-duped
-      {url: 'http://insecure.com/image2.jpeg', scheme: 'http', domain: 'insecure.com'},
-      {url: 'https://google.com/', scheme: 'https', domain: 'google.com'},
+      {url: 'https://google.com/', parsedURL: {scheme: 'https', host: 'google.com'}},


noisy changes to move off scheme and domain, which were just returning the parsedURL properties anyways

brendankenny · 2018-05-02T06:32:08Z

typings/artifacts.d.ts

-      requestLanternFirstCPUIdle(data: LH.Artifacts.MetricComputationData): Promise<Artifacts.LanternMetric>;
-      requestLanternFirstMeaningfulPaint(data: LH.Artifacts.MetricComputationData): Promise<Artifacts.LanternMetric>;
-      requestLanternSpeedIndex(data: LH.Artifacts.MetricComputationData): Promise<Artifacts.LanternMetric>;
+      requestLanternInteractive(data: LH.Artifacts.MetricComputationDataInput): Promise<Artifacts.LanternMetric>;


@patrickhulce I believe this was your intention all along (mostly because it works, and that can't be on accident :). I typed these as MetricComputationData based on their calls within the other metrics where the MetricComputationDataInput -> MetricComputationData transition had already taken place, but they of course work without the extra artifacts. This also makes predictive-perf.js work above.

I had to move the optional simulator to MetricComputationDataInput as a result

yes indeed the metric computation data is the internal one that gets augmented 👍

brendankenny · 2018-05-02T06:33:23Z

typings/artifacts.d.ts

@@ -314,6 +315,7 @@ declare global {

      export interface LanternMetric {
        timing: number;
+        timestamp?: never;


copied from #5084

paulirish · 2018-05-02T18:40:20Z

lighthouse-core/audits/is-on-https.js

           SECURE_SCHEMES.includes(record.protocol) ||
-           SECURE_DOMAINS.includes(record.domain);
+           SECURE_DOMAINS.includes(record.parsedURL.host);


you're 100% these two are the same?

you're 100% these two are the same?

yeah, both our version of the frontend and current master have domain a getter that returns this._parsedURL.host and scheme is this._parsedURL.scheme. Seems better to do this than duplicate (and in the spirit of "use the stable part of the API")

paulirish · 2018-05-02T18:42:56Z

lighthouse-core/test/results/sample_v2.json

@@ -292,7 +292,7 @@
    },
    "user-timings": {
      "score": 1,
-      "displayValue": "",
+      "displayValue": "0",


this does seem weird. can you avoid printing a 0 to the report plz :)
Might as well change it to "6 measures found"

can you avoid printing a 0 to the report plz :)

haha, that was my first instinct, but it seemed like too specific of a change to the audit. Done

paulirish · 2018-05-02T18:43:54Z

typings/externs.d.ts

@@ -144,6 +144,7 @@ declare global {
      tid: number;
      ts: number;
      dur: number;
+      ph: 'X'|'M'|'B'|'E'|'I'|'O'|'N'|'R'|'D'|'S'|'F';


there are others: https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview#heading=h.uxpopqvbjezh

patrickhulce

lgtm % nits

patrickhulce · 2018-05-02T20:21:35Z

lighthouse-core/audits/user-timings.js

@@ -78,32 +83,32 @@ class UserTimings extends Audit {
      ut.startTime = (ut.startTime - tabTrace.navigationStartEvt.ts) / 1000;
      if (!ut.isMark) {
        ut.endTime = (ut.endTime - tabTrace.navigationStartEvt.ts) / 1000;
-        ut.duration = ut.endTime - ut.startTime;
+        ut.duration = ut.duration / 1000;


just style?

just style?

yeah, had to set a duration when creating the object, and it seemed silly to set it to 0 only to set it to the actual duration a few lines down..

patrickhulce · 2018-05-02T20:22:16Z

lighthouse-core/audits/user-timings.js

      }
    });

    return userTimings;
  }

  /*
-   * @return {!Array<string>}
+   * @return {Array<string>}


patrickhulce · 2018-05-02T20:23:57Z

lighthouse-core/test/results/sample_v2.json

@@ -320,7 +320,7 @@
                "url": "http://localhost:10200/dobetterweb/dbw_tester.html",
                "startTime": 185603.321221,
                "endTime": 185603.961376,
-                "responseReceivedTime": 185603.89718499998,
+                "_responseReceivedTime": 185603.89718499998,


viewing these inline is a bit annoying it's the only _ 😆 We don't really want folks to use these though I guess

viewing these inline is a bit annoying

yeah, it seems like it really shouldn't be in the audit result anyways

patrickhulce · 2018-05-02T20:26:28Z

typings/artifacts.d.ts

-      requestLanternFirstCPUIdle(data: LH.Artifacts.MetricComputationData): Promise<Artifacts.LanternMetric>;
-      requestLanternFirstMeaningfulPaint(data: LH.Artifacts.MetricComputationData): Promise<Artifacts.LanternMetric>;
-      requestLanternSpeedIndex(data: LH.Artifacts.MetricComputationData): Promise<Artifacts.LanternMetric>;
+      requestLanternInteractive(data: LH.Artifacts.MetricComputationDataInput): Promise<Artifacts.LanternMetric>;


yes indeed the metric computation data is the internal one that gets augmented 👍

…ome#5090)

brendankenny requested review from patrickhulce and paulirish as code owners May 2, 2018 06:25

brendankenny commented May 2, 2018

View reviewed changes

brendankenny force-pushed the tsaudits3 branch from c81700d to 8d1c20e Compare May 2, 2018 06:36

paulirish approved these changes May 2, 2018

View reviewed changes

patrickhulce approved these changes May 2, 2018

View reviewed changes

brendankenny added 3 commits May 2, 2018 13:39

core(tsc): add type checking to remaining top-level audits

4ef453b

feedback

1420935

expectations

b4aa82a

brendankenny force-pushed the tsaudits3 branch from 07d273d to b4aa82a Compare May 2, 2018 20:39

feedback and lighthouse-core/audits/*.js

ba66953

brendankenny merged commit 26807e8 into master May 2, 2018

brendankenny deleted the tsaudits3 branch May 2, 2018 21:23

kdzwinel pushed a commit to kdzwinel/lighthouse that referenced this pull request Aug 16, 2018

core(tsc): add type checking to remaining top-level audits (GoogleChr…

d1ca7d5

…ome#5090)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(tsc): add type checking to remaining top-level audits #5090

core(tsc): add type checking to remaining top-level audits #5090

brendankenny commented May 2, 2018

brendankenny May 2, 2018

brendankenny May 2, 2018

brendankenny May 2, 2018

brendankenny May 2, 2018 •

edited

Loading

patrickhulce May 2, 2018

brendankenny May 2, 2018

paulirish May 2, 2018

brendankenny May 2, 2018

paulirish May 2, 2018

brendankenny May 2, 2018

paulirish May 2, 2018

brendankenny May 2, 2018

patrickhulce left a comment

patrickhulce May 2, 2018

brendankenny May 2, 2018

patrickhulce May 2, 2018

patrickhulce May 2, 2018

brendankenny May 2, 2018

patrickhulce May 2, 2018

core(tsc): add type checking to remaining top-level audits #5090

core(tsc): add type checking to remaining top-level audits #5090

Conversation

brendankenny commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny May 2, 2018 • edited Loading

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

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

brendankenny May 2, 2018 •

edited

Loading