Repository: GoogleChrome/lighthouse
Pull request #5849 (2018-08-16 19:59:37)

cli(tsc): make LH.Flags type correct and consistent
I've had a few TODOs sprinkled in the code to `fix Flags type`. I'm not exactly sure what I meant by that, but this PR makes sure our uses of `Flags` are all correct and gets to remove two places (`index.js` and `popup.js`) where we were coercing empty objects into Flags.

We allowed empty objects as flags there, and now the type system actually verifies that that's completely acceptable :)

We already had `LH.Flags` vs `LH.SharedFlagsSettings` (all properties shared by `LH.Flags` and `LH.Config.Settings`), and now I've added another level with `LH.Flags` vs `LH.CliFlags` (all properties *in addition* to `LH.Flags` that control just CLI functionality). This is another step in making `Flags` a little easier to reason about, because once they reach core, all those CLI flags aren't visible anymore (according to the type system). This means code really doesn't have to care if `Flags` originated from the CLI, someone calling the node module, the extension, or devtools.

Also spruces up type annotations in the CLI files (mostly removing `!`s), which haven't been touched in a while.


Patch:
diff --git a/lighthouse-cli/bin.js b/lighthouse-cli/bin.js
index b966aeaeb..4eb92521a 100644
--- a/lighthouse-cli/bin.js
+++ b/lighthouse-cli/bin.js
@@ -14,7 +14,6 @@ const getFlags = require('./cli-flags.js').getFlags;
 const runLighthouse = require('./run').runLighthouse;
 
 const log = require('lighthouse-logger');
-// @ts-ignore
 const pkg = require('../package.json');
 const Sentry = require('../lighthouse-core/lib/sentry');
 
@@ -31,7 +30,7 @@ function isDev() {
 // Tell user if there's a newer version of LH.
 updateNotifier({pkg}).notify();
 
-const /** @type {LH.Flags} */ cliFlags = getFlags();
+const cliFlags = getFlags();
 
 // Process terminating command
 if (cliFlags.listAllAudits) {
@@ -43,7 +42,6 @@ if (cliFlags.listTraceCategories) {
   commands.listTraceCategories();
 }
 
-/** @type {string} */
 const url = cliFlags._[0];
 
 /** @type {LH.Config.Json|undefined} */
@@ -79,11 +77,18 @@ if (
 }
 
 if (cliFlags.extraHeaders) {
-  if (cliFlags.extraHeaders.substr(0, 1) !== '{') {
-    cliFlags.extraHeaders = fs.readFileSync(cliFlags.extraHeaders, 'utf-8');
+  // TODO: LH.Flags.extraHeaders is actually a string at this point, but needs to be
+  // copied over to LH.Settings.extraHeaders, which is LH.Crdp.Network.Headers. Force
+  // the conversion here, but long term either the CLI flag or the setting should have
+  // a different name.
+  // @ts-ignore
+  let extraHeadersStr = /** @type {string} */ (cliFlags.extraHeaders);
+  // If not a JSON object, assume it's a path to a JSON file.
+  if (extraHeadersStr.substr(0, 1) !== '{') {
+    extraHeadersStr = fs.readFileSync(extraHeadersStr, 'utf-8');
   }
 
-  cliFlags.extraHeaders = JSON.parse(cliFlags.extraHeaders);
+  cliFlags.extraHeaders = JSON.parse(extraHeadersStr);
 }
 
 /**
diff --git a/lighthouse-cli/cli-flags.js b/lighthouse-cli/cli-flags.js
index 200d4c2c6..bf32293ec 100644
--- a/lighthouse-cli/cli-flags.js
+++ b/lighthouse-cli/cli-flags.js
@@ -8,13 +8,12 @@
 /* eslint-disable max-len */
 
 const yargs = require('yargs');
-// @ts-ignore
 const pkg = require('../package.json');
 const printer = require('./printer');
 
 /**
  * @param {string=} manualArgv
- * @return {!LH.Flags}
+ * @return {LH.CliFlags}
  */
 function getFlags(manualArgv) {
   // @ts-ignore yargs() is incorrectly typed as not returning itself
@@ -118,7 +117,7 @@ function getFlags(manualArgv) {
       // boolean values
       .boolean([
         'disable-storage-reset', 'disable-device-emulation', 'save-assets', 'list-all-audits',
-        'list-trace-categories', 'view', 'verbose', 'quiet', 'help',
+        'list-trace-categories', 'view', 'verbose', 'quiet', 'help', 'enable-error-reporting',
       ])
       .choices('output', printer.getValidOutputOptions())
       .choices('throttling-method', ['devtools', 'provided', 'simulate'])
@@ -137,7 +136,7 @@ function getFlags(manualArgv) {
       .default('output', ['html'])
       .default('port', 0)
       .default('hostname', 'localhost')
-      .check(/** @param {!LH.Flags} argv */ (argv) => {
+      .check(/** @param {LH.CliFlags} argv */ (argv) => {
         // Lighthouse doesn't need a URL if...
         //   - We're in auditMode (and we have artifacts already)
         //   - We're just listing the available options.
diff --git a/lighthouse-cli/printer.js b/lighthouse-cli/printer.js
index ac6bc867b..e41e30324 100644
--- a/lighthouse-cli/printer.js
+++ b/lighthouse-cli/printer.js
@@ -13,6 +13,7 @@ const log = require('lighthouse-logger');
  *   'json': JSON formatted results
  *   'html': An HTML report
  *   'csv': CSV formatted results
+ * @type {SelfMap<LH.OutputMode>}
  */
 const OutputMode = {
   json: 'json',
@@ -36,7 +37,7 @@ function checkOutputPath(path) {
 /**
  * Writes the output to stdout.
  * @param {string} output
- * @return {!Promise<undefined>}
+ * @return {Promise<void>}
  */
 function writeToStdout(output) {
   return new Promise(resolve => {
@@ -52,8 +53,8 @@ function writeToStdout(output) {
  * Writes the output to a file.
  * @param {string} filePath
  * @param {string} output
- * @param {string} outputMode
- * @return {!Promise<undefined>}
+ * @param {LH.OutputMode} outputMode
+ * @return {Promise<void>}
  */
 function writeFile(filePath, output, outputMode) {
   return new Promise((resolve, reject) => {
@@ -71,7 +72,7 @@ function writeFile(filePath, output, outputMode) {
 /**
  * Writes the output.
  * @param {string} output
- * @param {string} mode
+ * @param {LH.OutputMode} mode
  * @param {string} path
  * @return {Promise<void>}
  */
@@ -84,7 +85,7 @@ async function write(output, mode, path) {
 
 /**
  * Returns a list of valid output options.
- * @return {!Array<string>}
+ * @return {Array<string>}
  */
 function getValidOutputOptions() {
   return Object.keys(OutputMode);
diff --git a/lighthouse-cli/run.js b/lighthouse-cli/run.js
index fe84f7396..6a55fe5f5 100644
--- a/lighthouse-cli/run.js
+++ b/lighthouse-cli/run.js
@@ -26,7 +26,7 @@ const _PROTOCOL_TIMEOUT_EXIT_CODE = 67;
 /**
  * exported for testing
  * @param {string} flags
- * @return {!Array<string>}
+ * @return {Array<string>}
  */
 function parseChromeFlags(flags = '') {
   const parsed = yargsParser(
@@ -49,8 +49,8 @@ function parseChromeFlags(flags = '') {
 /**
  * Attempts to connect to an instance of Chrome with an open remote-debugging
  * port. If none is found, launches a debuggable instance.
- * @param {!LH.Flags} flags
- * @return {Promise<LH.LaunchedChrome>}
+ * @param {LH.CliFlags} flags
+ * @return {Promise<ChromeLauncher.LaunchedChrome>}
  */
 function getDebuggableChrome(flags) {
   return ChromeLauncher.launch({
@@ -71,7 +71,7 @@ function showProtocolTimeoutError() {
 }
 
 /**
- * @param {!LH.LighthouseError} err
+ * @param {LH.LighthouseError} err
  */
 function showRuntimeError(err) {
   console.error('Runtime error encountered:', err.friendlyMessage || err.message);
@@ -82,7 +82,7 @@ function showRuntimeError(err) {
 }
 
 /**
- * @param {!LH.LighthouseError} err
+ * @param {LH.LighthouseError} err
  */
 function handleError(err) {
   if (err.code === 'ECONNREFUSED') {
@@ -95,8 +95,8 @@ function handleError(err) {
 }
 
 /**
- * @param {!LH.RunnerResult} runnerResult
- * @param {!LH.Flags} flags
+ * @param {LH.RunnerResult} runnerResult
+ * @param {LH.CliFlags} flags
  * @return {Promise<void>}
  */
 async function saveResults(runnerResult, flags) {
@@ -138,12 +138,12 @@ async function saveResults(runnerResult, flags) {
 
 /**
  * @param {string} url
- * @param {LH.Flags} flags
+ * @param {LH.CliFlags} flags
  * @param {LH.Config.Json|undefined} config
  * @return {Promise<LH.RunnerResult|void>}
  */
 function runLighthouse(url, flags, config) {
-  /** @type {!LH.LaunchedChrome} */
+  /** @type {ChromeLauncher.LaunchedChrome|undefined} */
   let launchedChrome;
   const shouldGather = flags.gatherMode || flags.gatherMode === flags.auditMode;
   let chromeP = Promise.resolve();
diff --git a/lighthouse-cli/sentry-prompt.js b/lighthouse-cli/sentry-prompt.js
index 71d7d7c86..405377e1d 100644
--- a/lighthouse-cli/sentry-prompt.js
+++ b/lighthouse-cli/sentry-prompt.js
@@ -18,7 +18,7 @@ const MESSAGE = `${log.reset}We're constantly trying to improve Lighthouse and i
   ` May we anonymously report runtime exceptions to improve the tool over time? `;
 
 /**
- * @return {!Promise<boolean>}
+ * @return {Promise<boolean>}
  */
 function prompt() {
   if (!process.stdout.isTTY || process.env.CI) {
@@ -58,7 +58,7 @@ function prompt() {
 }
 
 /**
- * @return {!Promise<boolean>}
+ * @return {Promise<boolean>}
  */
 function askPermission() {
   return Promise.resolve().then(_ => {
diff --git a/lighthouse-core/audits/metrics/first-contentful-paint.js b/lighthouse-core/audits/metrics/first-contentful-paint.js
index bf5ad01c4..c299f4392 100644
--- a/lighthouse-core/audits/metrics/first-contentful-paint.js
+++ b/lighthouse-core/audits/metrics/first-contentful-paint.js
@@ -12,8 +12,8 @@ const UIStrings = {
   /** The name of the metric that marks the time at which the first text or image is painted by the browser. Shown to users as the label for the numeric metric value. Ideally fits within a ~40 character limit. */
   title: 'First Contentful Paint',
   /** Description of the First Contentful Paint (FCP) metric, which marks the time at which the first text or image is painted by the browser. This is displayed within a tooltip when the user hovers on the metric name to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */
-  description: 'First contentful paint marks the time at which the first text or image is ' +
-      `painted. [Learn more](https://developers.google.com/web/fundamentals/performance/user-centric-performance-metrics#first_paint_and_first_contentful_paint).`,
+  description: 'First Contentful Paint marks the time at which the first text or image is ' +
+      `painted. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/first-contentful-paint).`,
 };
 
 const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);
diff --git a/lighthouse-core/config/config.js b/lighthouse-core/config/config.js
index d84d81f38..c10975d47 100644
--- a/lighthouse-core/config/config.js
+++ b/lighthouse-core/config/config.js
@@ -160,18 +160,18 @@ function assertValidGatherer(gathererInstance, gathererName) {
 /**
  * Creates a settings object from potential flags object by dropping all the properties
  * that don't exist on Config.Settings.
- * TODO(bckenny): fix Flags type
  * @param {Partial<LH.Flags>=} flags
- * @return {Partial<LH.Config.Settings>}
- */
+ * @return {RecursivePartial<LH.Config.Settings>}
+*/
 function cleanFlagsForSettings(flags = {}) {
-  /** @type {Partial<LH.Config.Settings>} */
+  /** @type {RecursivePartial<LH.Config.Settings>} */
   const settings = {};
 
   for (const key of Object.keys(flags)) {
     // @ts-ignore - intentionally testing some keys not on defaultSettings to discard them.
     if (typeof constants.defaultSettings[key] !== 'undefined') {
-      const safekey = /** @type {keyof LH.SharedFlagsSettings} */ (key);
+      // Cast since key now must be able to index both Flags and Settings.
+      const safekey = /** @type {Extract<keyof LH.Flags, keyof LH.Config.Settings>} */ (key);
       settings[safekey] = flags[safekey];
     }
   }
diff --git a/lighthouse-core/index.js b/lighthouse-core/index.js
index 4a639c815..1d2ddb3b8 100644
--- a/lighthouse-core/index.js
+++ b/lighthouse-core/index.js
@@ -31,10 +31,7 @@ const Config = require('./config/config');
  * @param {LH.Config.Json=} configJSON
  * @return {Promise<LH.RunnerResult|undefined>}
  */
-async function lighthouse(url, flags, configJSON) {
-  // TODO(bckenny): figure out Flags types.
-  flags = flags || /** @type {LH.Flags} */ ({});
-
+async function lighthouse(url, flags = {}, configJSON) {
   // set logging preferences, assume quiet
   flags.logLevel = flags.logLevel || 'error';
   log.setLevel(flags.logLevel);
diff --git a/lighthouse-core/lib/locales/en-US.json b/lighthouse-core/lib/locales/en-US.json
index 6e61b4a2f..15e947076 100644
--- a/lighthouse-core/lib/locales/en-US.json
+++ b/lighthouse-core/lib/locales/en-US.json
@@ -224,7 +224,7 @@
     "description": "The name of the metric that marks the estimated time between the page receiving input (a user clicking, tapping, or typing) and the page responding. Shown to users as the label for the numeric metric value. Ideally fits within a ~40 character limit."
   },
   "lighthouse-core/audits/metrics/first-contentful-paint.js | description": {
-    "message": "First contentful paint marks the time at which the first text or image is painted. [Learn more](https://developers.google.com/web/fundamentals/performance/user-centric-performance-metrics#first_paint_and_first_contentful_paint).",
+    "message": "First Contentful Paint marks the time at which the first text or image is painted. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/first-contentful-paint).",
     "description": "Description of the First Contentful Paint (FCP) metric, which marks the time at which the first text or image is painted by the browser. This is displayed within a tooltip when the user hovers on the metric name to see more. No character length limits. 'Learn More' becomes link text to additional documentation."
   },
   "lighthouse-core/audits/metrics/first-contentful-paint.js | title": {
diff --git a/lighthouse-extension/app/src/popup.js b/lighthouse-extension/app/src/popup.js
index 09eaa3212..e644c9f7a 100644
--- a/lighthouse-extension/app/src/popup.js
+++ b/lighthouse-extension/app/src/popup.js
@@ -160,8 +160,8 @@ async function onGenerateReportButtonClick(background, settings) {
   feedbackEl.textContent = '';
 
   const {selectedCategories, useDevTools} = settings;
-  // TODO(bckenny): make flags workable as a type.
-  const flags = /** @type {LH.Flags} */ ({throttlingMethod: useDevTools ? 'devtools' : 'simulate'});
+  /** @type {LH.Flags} */
+  const flags = {throttlingMethod: useDevTools ? 'devtools' : 'simulate'};
 
   try {
     await background.runLighthouseInExtension(flags, selectedCategories);
diff --git a/typings/externs.d.ts b/typings/externs.d.ts
index ac43aba8b..da1aca029 100644
--- a/typings/externs.d.ts
+++ b/typings/externs.d.ts
@@ -28,6 +28,18 @@ declare global {
     [P in K]+?: T[P]
   }
 
+  /** An object with the keys in the union K mapped to themselves as values. */
+  type SelfMap<K extends string> = {
+    [P in K]: P;
+  };
+
+  /** Make optional all properties on T and any properties on object properties of T. */
+  type RecursivePartial<T> = {
+    [P in keyof T]+?: T[P] extends object ?
+      RecursivePartial<T[P]> :
+      T[P];
+  };
+
   /**
    * Exclude void from T
    */
@@ -77,16 +89,21 @@ declare global {
       onlyAudits?: string[] | null;
       onlyCategories?: string[] | null;
       skipAudits?: string[] | null;
+      extraHeaders?: Crdp.Network.Headers | null; // See extraHeaders TODO in bin.js
     }
 
     export interface Flags extends SharedFlagsSettings {
-      // Used by both core/ and cli/
-      port: number;
-      hostname: string;
-      output: any;
-      logLevel: 'silent'|'error'|'info'|'verbose';
+      port?: number;
+      hostname?: string;
+      logLevel?: 'silent'|'error'|'info'|'verbose';
+      configPath?: string;
+    }
 
-      // Just used by cli/
+    /**
+     * Flags accepted by Lighthouse, plus additional flags just
+     * for controlling the CLI.
+     */
+    export interface CliFlags extends Flags {
       _: string[];
       chromeFlags: string;
       outputPath: string;
@@ -95,11 +112,13 @@ declare global {
       enableErrorReporting: boolean;
       listAllAudits: boolean;
       listTraceCategories: boolean;
-      configPath?: string;
       preset?: 'full'|'mixed-content'|'perf';
       verbose: boolean;
       quiet: boolean;
-      extraHeaders?: string;
+      // following are given defaults in cli-flags, so not optional like in Flags or SharedFlagsSettings
+      output: OutputMode[];
+      port: number;
+      hostname: string;
     }
 
     export interface RunnerResult {
@@ -120,12 +139,6 @@ declare global {
       group: string;
     }
 
-    export interface LaunchedChrome {
-      pid: number;
-      port: number;
-      kill: () => Promise<{}>;
-    }
-
     export interface LighthouseError extends Error {
       friendlyMessage?: string;
     }


Test patch:
diff --git a/lighthouse-cli/test/cli/run-test.js b/lighthouse-cli/test/cli/run-test.js
index ec911a84e..c60addf08 100644
--- a/lighthouse-cli/test/cli/run-test.js
+++ b/lighthouse-cli/test/cli/run-test.js
@@ -70,10 +70,6 @@ describe('Parsing --chrome-flags', () => {
     assert.deepStrictEqual(parseChromeFlags('--debug=false'), ['--debug=false']);
   });
 
-  it('returns empty when passed undefined', () => {
-    assert.deepStrictEqual(parseChromeFlags(), []);
-  });
-
   it('keeps --no-flags untouched, #3003', () => {
     assert.deepStrictEqual(parseChromeFlags('--no-sandbox'), ['--no-sandbox']);
   });
diff --git a/lighthouse-core/test/results/sample_v2.json b/lighthouse-core/test/results/sample_v2.json
index a65cef818..b31e7baa2 100644
--- a/lighthouse-core/test/results/sample_v2.json
+++ b/lighthouse-core/test/results/sample_v2.json
@@ -75,7 +75,7 @@
     "first-contentful-paint": {
       "id": "first-contentful-paint",
       "title": "First Contentful Paint",
-      "description": "First contentful paint marks the time at which the first text or image is painted. [Learn more](https://developers.google.com/web/fundamentals/performance/user-centric-performance-metrics#first_paint_and_first_contentful_paint).",
+      "description": "First Contentful Paint marks the time at which the first text or image is painted. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/first-contentful-paint).",
       "score": 0.51,
       "scoreDisplayMode": "numeric",
       "rawValue": 3969.135,