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.

Sign up for GitHub

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

Jump to bottom

cli(tsc): make LH.Flags type correct and consistent #5849

Merged
merged 2 commits into from
Aug 17, 2018
Merged

cli(tsc): make LH.Flags type correct and consistent #5849

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
01d558e
core(tsc): make LH.Flags type correct
brendankenny Aug 16, 2018
ef3523b
default enableErrorReporting
brendankenny Aug 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions lighthouse-cli/bin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand All @@ -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) {
Expand All @@ -43,7 +42,6 @@ if (cliFlags.listTraceCategories) {
commands.listTraceCategories();
}

/** @type {string} */
const url = cliFlags._[0];

/** @type {LH.Config.Json|undefined} */
Expand Down Expand Up @@ -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);
}

/**
Expand Down
5 changes: 2 additions & 3 deletions lighthouse-cli/cli-flags.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
11 changes: 6 additions & 5 deletions lighthouse-cli/printer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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 => {
Expand All @@ -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) => {
Expand All @@ -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>}
*/
Expand All @@ -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);
Expand Down
18 changes: 9 additions & 9 deletions lighthouse-cli/run.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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({
Expand All @@ -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);
Expand All @@ -82,7 +82,7 @@ function showRuntimeError(err) {
}

/**
* @param {!LH.LighthouseError} err
* @param {LH.LighthouseError} err
*/
function handleError(err) {
if (err.code === 'ECONNREFUSED') {
Expand All @@ -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) {
Expand Down Expand Up @@ -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();
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-cli/sentry-prompt.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -58,7 +58,7 @@ function prompt() {
}

/**
* @return {!Promise<boolean>}
* @return {Promise<boolean>}
*/
function askPermission() {
return Promise.resolve().then(_ => {
Expand Down
4 changes: 0 additions & 4 deletions lighthouse-cli/test/cli/run-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']);
});
Expand Down
10 changes: 5 additions & 5 deletions lighthouse-core/config/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>}
Copy link
Member Author

Choose a reason for hiding this comment

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

It's a bug that this wasn't flagged before, but it wasn't until I made any change to LH.Flags.output that it started to be flagged (even though the problem is with LH.Flags.throttling, weirdly enough).

We just want to copy any Settings properties over from an instance of Flags, so the result will only have Settings properties but some may not be set. So Partial<LH.Config.Settings> would be correct if Settings were a simple object.

The issue is that one of the properties (throttling) is itself an object, and while Flags.throttling has all optional properties, on Settings.throttling they're all required. This change applies Partial<> to all of Settings properties recursively. It could special case throttling instead, but this will catch any future objects added to SharedFlagsSettings as well.

*/
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];
}
}
Expand Down
5 changes: 1 addition & 4 deletions lighthouse-core/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-extension/app/src/popup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
43 changes: 28 additions & 15 deletions typings/externs.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -77,29 +89,36 @@ 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;
saveAssets: boolean;
view: boolean;
enableErrorReporting: boolean;
enableErrorReporting?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

make this a yargs boolean instead. seems weird to have this one optional but the rest not.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

wait noooooo, we gotta revert this. it's purposefully not a yargs boolean so that the setting saved to their config will be respected. explicitly setting as false via yargs will always turn error reporting off

Copy link
Member Author

Choose a reason for hiding this comment

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

whoops, very good point. There was a reason it wasn't in yargs.boolean() already :) I'll revert and add a comment so we don't do that again

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 {
Expand All @@ -120,12 +139,6 @@ declare global {
group: string;
}

export interface LaunchedChrome {
pid: number;
port: number;
kill: () => Promise<{}>;
}

export interface LighthouseError extends Error {
friendlyMessage?: string;
}
Expand Down