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.

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

extension(tsc): add type checking to extension entry points #5346

Merged
merged 8 commits into from
May 29, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
9 changes: 7 additions & 2 deletions lighthouse-cli/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,13 @@ function runLighthouse(url, flags, config) {
const resultsP = chromeP.then(_ => {
return lighthouse(url, flags, config).then(runnerResult => {
return potentiallyKillChrome().then(_ => runnerResult);
}).then(runnerResult => {
return saveResults(runnerResult, flags).then(_ => runnerResult);
}).then(async runnerResult => {
// If in gatherMode only, there will be no runnerResult.
if (runnerResult) {
await saveResults(runnerResult, flags);
}

return runnerResult;
});
});

Expand Down
19 changes: 4 additions & 15 deletions lighthouse-core/gather/connections/raw.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,11 @@ const Connection = require('./connection.js');
/* eslint-disable no-unused-vars */

/**
* @interface
Copy link
Member Author

Choose a reason for hiding this comment

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

turns out this was never working as an interface for tsc (@interface has closure support only). Making a typedef so can be imported elsewhere

* @typedef {object} Port
* @property {(eventName: 'message'|'close', cb: ((arg: string) => void) | (() => void)) => void} on
* @property {(message: string) => void} send
* @property {() => void} close
*/
class Port {
/**
* @param {'message' | 'close'} eventName
* @param {function(string)|function()} cb
*/
on(eventName, cb) { }

/**
* @param {string} message
*/
send(message) { }

close() { }
}

/* eslint-enable no-unused-vars */

Expand Down
8 changes: 4 additions & 4 deletions lighthouse-core/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
* 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
'use strict';

const Runner = require('./runner');
Expand All @@ -30,16 +29,17 @@ const Config = require('./config/config');
* @param {string} url
* @param {LH.Flags} flags
* @param {LH.Config.Json|undefined} configJSON
* @return {Promise<LH.RunnerResult>}
* @return {Promise<LH.RunnerResult|undefined>}
*/
function lighthouse(url, flags = {}, configJSON) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a breaking change for lighthouse('http://example.com') in node, do we really need to do this?

Copy link
Member Author

Choose a reason for hiding this comment

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

do we support fully optional flags? I hadn't dug into it so I didn't pursue it ({} isn't currently castable to LH.Flags which is why I dropped it), but if so, agreed we should support that here.

I can add a manual cast of a {} here, maybe, and then follow up with solidifying the flags pipeline in a follow up?

Copy link
Collaborator

Choose a reason for hiding this comment

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

would Partial<LH.Flags> cut it? I guess we could differentiate LH.Flags from LH.CLIFlags which is basically what's defined now

Copy link
Member Author

Choose a reason for hiding this comment

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

would Partial<LH.Flags> cut it? I guess we could differentiate LH.Flags from LH.CLIFlags which is basically what's defined now

yeah, I think that's basically what we need to do. Handy to have output from yargs, but then our optional flags fed into Config is a different beast (and it looks like completely optional is fine).

Ok if we do a future "Make Flags Good" PR while leaving this with the {} default?

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

function lighthouse(url, flags, configJSON) {
return Promise.resolve().then(_ => {
// set logging preferences, assume quiet
flags.logLevel = flags.logLevel || 'error';
log.setLevel(flags.logLevel);

// Use ConfigParser to generate a valid config file
const config = new Config(configJSON, flags);
// @ts-ignore - TODO(bckenny): type checking for Config
const config = /** @type {LH.Config} */ (new Config(configJSON, flags));
const connection = new ChromeProtocol(flags.port, flags.hostname);

// kick off a lighthouse run
Expand Down
1 change: 0 additions & 1 deletion lighthouse-extension/app/manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
"default_locale": "en",
"background": {
"scripts": [
"scripts/chromereload.js",
"scripts/lighthouse-ext-background.js"
],
"persistent": false
Expand Down
1 change: 0 additions & 1 deletion lighthouse-extension/app/manifest_canary.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
"default_locale": "en",
"background": {
"scripts": [
"scripts/chromereload.js",
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we just delete this are we using manifest canary?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm good with deleting, but we should probably delete the actual extension in that case? @paulirish

Copy link
Member

@paulirish paulirish May 24, 2018

Choose a reason for hiding this comment

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

ill delete canary extension now
edit: done.

"scripts/lighthouse-ext-background.js"
],
"persistent": false
Expand Down
6 changes: 0 additions & 6 deletions lighthouse-extension/app/popup.html
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,6 @@ <h2 class="header-titles__url">...</h2>

<aside class="options subpage">
<h2 class="options__title" hidden>Settings</h2>
<div hidden>
<label>
<input type="checkbox" class="setting-disable-extensions" disabled
value="Disable other extensions while Lighthouse audits a page">Disable other extensions while Lighthouse audits a page.
</label>
</div>

<h2 class="options__title">Audit categories to include</h2>
<ul class="options__list">
Expand Down
29 changes: 0 additions & 29 deletions lighthouse-extension/app/src/chromereload.js

This file was deleted.

64 changes: 42 additions & 22 deletions lighthouse-extension/app/src/lighthouse-background.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,24 @@ const Config = require('../../../lighthouse-core/config/config');
const defaultConfig = require('../../../lighthouse-core/config/default-config.js');
const log = require('lighthouse-logger');

/** @typedef {import('../../../lighthouse-core/gather/connections/connection.js')} Connection */

/**
* @param {!Connection} connection
* @param {Connection} connection
* @param {string} url
* @param {!Object} options Lighthouse options.
* @param {!Array<string>} categoryIDs Name values of categories to include.
* @return {!Promise}
* @param {{flags: LH.Flags}} options Lighthouse options.
* @param {Array<string>} categoryIDs Name values of categories to include.
* @param {(url?: string) => void} updateBadgeFn
* @return {Promise<LH.RunnerResult|void>}
*/
window.runLighthouseForConnection = function(
connection, url, options, categoryIDs,
updateBadgeFn = function() { }) {
const config = new Config({
function runLighthouseForConnection(
connection, url, options, categoryIDs,
updateBadgeFn = function() { }) {
// @ts-ignore - TODO(bckenny): type checking for Config
const config = /** @type {LH.Config} */ (new Config({
extends: 'lighthouse:default',
settings: {onlyCategories: categoryIDs},
}, options.flags);
}, options.flags));

// Add url and config to fresh options object.
const runOptions = Object.assign({}, options, {url, config});
Expand All @@ -39,30 +43,46 @@ window.runLighthouseForConnection = function(
updateBadgeFn();
throw err;
});
};
}

/**
* @param {!RawProtocol.Port} port
* @param {RawProtocol.Port} port
* @param {string} url
* @param {!Object} options Lighthouse options.
* @param {!Array<string>} categoryIDs Name values of categories to include.
* @return {!Promise}
* @param {{flags: LH.Flags}} options Lighthouse options.
* @param {Array<string>} categoryIDs Name values of categories to include.
* @return {Promise<LH.RunnerResult|void>}
*/
window.runLighthouseInWorker = function(port, url, options, categoryIDs) {
function runLighthouseInWorker(port, url, options, categoryIDs) {
// Default to 'info' logging level.
log.setLevel('info');
const connection = new RawProtocol(port);
return window.runLighthouseForConnection(connection, url, options, categoryIDs);
};
return runLighthouseForConnection(connection, url, options, categoryIDs);
}

/**
* Returns list of top-level categories from the default config.
* @return {!Array<{title: string, id: string}>}
* @return {Array<{title: string, id: string}>}
*/
window.getDefaultCategories = function() {
function getDefaultCategories() {
return Config.getCategories(defaultConfig);
};
}

window.listenForStatus = function(listenCallback) {
/** @param {(status: [string, string, string]) => void} listenCallback */
function listenForStatus(listenCallback) {
log.events.addListener('status', listenCallback);
};
}

if (typeof module !== 'undefined' && module.exports) {
Copy link
Member

Choose a reason for hiding this comment

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

really dig this. thanks.

module.exports = {
runLighthouseForConnection,
runLighthouseInWorker,
getDefaultCategories,
listenForStatus,
};
} else {
// If not require()d, expose on window for devtools, other consumers of file.
// @ts-ignore
window.runLighthouseInWorker = runLighthouseInWorker;
// @ts-ignore
window.listenForStatus = listenForStatus;
}
Loading