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

core(tsc): update to latest tsc #5581

Merged
merged 3 commits into from
Aug 8, 2018
Merged
Show file tree
Hide file tree
Changes from all 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
53 changes: 28 additions & 25 deletions lighthouse-core/gather/connections/connection.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ const LHError = require('../../lib/errors');

/**
* @typedef {LH.StrictEventEmitter<{'protocolevent': LH.Protocol.RawEventMessage}>} CrdpEventMessageEmitter
* @typedef {LH.CrdpCommands[keyof LH.CrdpCommands]['paramsType']} CommandParamsTypes
* @typedef {LH.CrdpCommands[keyof LH.CrdpCommands]['returnType']} CommandReturnTypes
* @typedef {LH.CrdpCommands[keyof LH.CrdpCommands]} CommandInfo
* @typedef {{resolve: function(Promise<CommandInfo['returnType']>): void, method: keyof LH.CrdpCommands, options: {silent?: boolean}}} CommandCallback
*/

class Connection {
constructor() {
this._lastCommandId = 0;
/** @type {Map<number, {resolve: function(Promise<CommandReturnTypes>), method: keyof LH.CrdpCommands, options: {silent?: boolean}}>} */
/** @type {Map<number, CommandCallback>} */
this._callbacks = new Map();

/** @type {?CrdpEventMessageEmitter} */
Expand Down Expand Up @@ -46,6 +46,22 @@ class Connection {
return Promise.reject(new Error('Not implemented'));
}

/**
* Bind listeners for connection events.
* @param {'protocolevent'} eventName
* @param {function(LH.Protocol.RawEventMessage): void} cb
*/
on(eventName, cb) {
if (eventName !== 'protocolevent') {
throw new Error('Only supports "protocolevent" events');
}

if (!this._eventEmitter) {
throw new Error('Attempted to add event listener after connection disposed.');
}
this._eventEmitter.on(eventName, cb);
}

/* eslint-disable no-unused-vars */

/**
Expand All @@ -68,11 +84,9 @@ class Connection {

// Responses to commands carry "id" property, while events do not.
if (!('id' in object)) {
// tsc doesn't currently narrow type in !in branch, so manually cast.
const eventMessage = /** @type {LH.Protocol.RawEventMessage} */(object);
log.formatProtocol('<= event',
{method: eventMessage.method, params: eventMessage.params}, 'verbose');
this.emitProtocolEvent(eventMessage);
{method: object.method, params: object.params}, 'verbose');
this.emitProtocolEvent(object);
return;
}

Expand Down Expand Up @@ -124,29 +138,18 @@ class Connection {
}
}

// Declared outside class body because function expressions can be typed via more expressive @type
/**
* Bind listeners for connection events
* @type {CrdpEventMessageEmitter['on']}
*/
Connection.prototype.on = function on(eventName, cb) {
if (eventName !== 'protocolevent') {
throw new Error('Only supports "protocolevent" events');
}

if (!this._eventEmitter) {
throw new Error('Attempted to add event listener after connection disposed.');
}
this._eventEmitter.on(eventName, cb);
};

// Declared outside class body because function expressions can be typed via coercive @type
/**
* Looser-typed internal implementation of `Connection.sendCommand` which is
* strictly typed externally on exposed Connection interface. See
* `Driver.sendCommand` for explanation.
* @type {(this: Connection, method: keyof LH.CrdpCommands, params?: CommandParamsTypes, cmdOpts?: {silent?: boolean}) => Promise<CommandReturnTypes>}
* @this {Connection}
* @param {keyof LH.CrdpCommands} method
* @param {CommandInfo['paramsType']=} params,
* @param {{silent?: boolean}=} cmdOpts
* @return {Promise<CommandInfo['returnType']>}
*/
function _sendCommand(method, params = {}, cmdOpts = {}) {
function _sendCommand(method, params, cmdOpts = {}) {
/* eslint-disable no-invalid-this */
log.formatProtocol('method => browser', {method, params}, 'verbose');
const id = ++this._lastCommandId;
Expand Down
14 changes: 8 additions & 6 deletions lighthouse-core/gather/connections/extension.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,17 +173,19 @@ class ExtensionConnection extends Connection {
}

/**
* @typedef {LH.CrdpCommands[keyof LH.CrdpCommands]['paramsType']} CommandParamsTypes
* @typedef {LH.CrdpCommands[keyof LH.CrdpCommands]['returnType']} CommandReturnTypes
* @typedef {LH.CrdpCommands[keyof LH.CrdpCommands]} CommandInfo
*/
// Declared outside class body because function expressions can be typed via more expressive @type
// Declared outside class body because function expressions can be typed via coercive @type
/**
* Looser-typed internal implementation of `ExtensionConnection.sendCommand`
* which is strictly typed externally on exposed ExtensionConnection interface.
* See `Driver.sendCommand` for explanation.
* @type {(this: ExtensionConnection, method: keyof LH.CrdpCommands, params?: CommandParamsTypes) => Promise<CommandReturnTypes>}
* @this {ExtensionConnection}
* @param {keyof LH.CrdpCommands} method
* @param {CommandInfo['paramsType']=} params,
* @return {Promise<CommandInfo['returnType']>}
*/
function _sendCommand(method, params = {}) {
function _sendCommand(method, params) {
return new Promise((resolve, reject) => {
log.formatProtocol('method => browser', {method, params}, 'verbose');
if (!this._tabId) { // eslint-disable-line no-invalid-this
Expand All @@ -192,7 +194,7 @@ function _sendCommand(method, params = {}) {
}

// eslint-disable-next-line no-invalid-this
chrome.debugger.sendCommand({tabId: this._tabId}, method, params, result => {
chrome.debugger.sendCommand({tabId: this._tabId}, method, params || {}, result => {
if (chrome.runtime.lastError) {
// The error from the extension has a `message` property that is the
// stringified version of the actual protocol error object.
Expand Down
8 changes: 6 additions & 2 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -1167,9 +1167,13 @@ Driver.prototype.off = function off(eventName, cb) {
* Necessitated by `params` only being optional for some values of `method`.
* See https://github.com/Microsoft/TypeScript/issues/5453 for needed variadic
* primitive.
* @type {(this: Driver, method: any, params?: any, cmdOpts?: {silent?: boolean}) => Promise<CommandReturnTypes>}
* @this {Driver}
* @param {any} method
* @param {any=} params,
* @param {{silent?: boolean}=} cmdOpts
* @return {Promise<CommandReturnTypes>}
*/
function _sendCommand(method, params = {}, cmdOpts = {}) {
function _sendCommand(method, params, cmdOpts = {}) {
const domainCommand = /^(\w+)\.(enable|disable)$/.exec(method);
if (domainCommand) {
const enable = domainCommand[2] === 'enable';
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/lib/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ const ERRORS = {

Object.keys(ERRORS).forEach(code => ERRORS[code].code = code);

/** @type {Object<keyof ERRORS, LighthouseErrorDefinition>} */
/** @type {Record<keyof typeof ERRORS, LighthouseErrorDefinition>} */
LighthouseError.errors = ERRORS;
module.exports = LighthouseError;

1 change: 1 addition & 0 deletions lighthouse-core/lib/locales/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
/** @type {Record<LH.Locale, LocaleMessages>} */
const locales = {
'en-US': require('./en-US.json'), // The 'source' strings, with descriptions
// @ts-ignore - tsc bug, something about en/en-US pointing to same file
Copy link
Member Author

Choose a reason for hiding this comment

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

this is a really weird bug. Any characters changed or added to either the en or en-US keys makes the error go away

'en': require('./en-US.json'), // According to CLDR/ICU, 'en' == 'en-US' dates/numbers (Why?!)

// TODO: en-GB has just ~10 messages that are different from en-US. We should only ship those.
Expand Down
12 changes: 6 additions & 6 deletions lighthouse-extension/app/src/lighthouse-ext-background.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,9 @@ async function runLighthouseInExtension(flags, categoryIDs) {
throw new Error('no runnerResult generated by Lighthouse');
}

const blobURL = createReportPageAsBlob(runnerResult);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why does ts require us to do this 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.

runnerResult.report is string | Array<string>, but we were treating it as a string (which it is in this case, but the compiler doesn't know that). I'm not sure why it wasn't catching it before.

// Report is always a singular string since {output: 'html'}, above.
const reportHtml = /** @type {string} */ (runnerResult.report);
const blobURL = createReportPageAsBlob(reportHtml);
await new Promise(resolve => chrome.windows.create({url: blobURL}, resolve));
}

Expand All @@ -91,15 +93,13 @@ async function runLighthouseAsInCLI(connection, url, flags, categoryIDs, {logAss
return results.report;
}


/**
* @param {LH.RunnerResult} runnerResult Lighthouse results object
* @param {string} reportHtml
* @return {string} Blob URL of the report (or error page) HTML
*/
function createReportPageAsBlob(runnerResult) {
function createReportPageAsBlob(reportHtml) {
performance.mark('report-start');
const html = runnerResult.report;
const blob = new Blob([html], {type: 'text/html'});
const blob = new Blob([reportHtml], {type: 'text/html'});
const blobURL = URL.createObjectURL(blob);

performance.mark('report-end');
Expand Down
8 changes: 7 additions & 1 deletion lighthouse-viewer/app/src/lighthouse-report-viewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,13 @@ class LighthouseReportViewer {
return new Promise((resolve, reject) => {
const reader = new FileReader();
reader.onload = function(e) {
resolve(e.target && e.target.result);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why? is new ts stricter on 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.

technically it's somewhat more correct. They had a custom FileReaderLoadEvent passed to onload before, which always has the target set to the FileReader, but that isn't a real standardized event. They're moving to automatically generate the types from Edge's Web IDL files, so I think that's why that custom event was eliminated.

However, the event should be generic, parameterized on the element itself so that the type is known and it isn't just an EventTarget but the actual element, but they haven't done that. Sort of for performance reasons, but probably also partly because people aren't asking hard enough: microsoft/TypeScript-DOM-lib-generator#207

const readerTarget = /** @type {?FileReader} */ (e.target);
const result = /** @type {?string} */ (readerTarget && readerTarget.result);
if (!result) {
reject('Could not read file');
return;
}
resolve(result);
};
reader.onerror = reject;
reader.readAsText(file);
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@
"pretty-json-stringify": "^0.0.2",
"puppeteer": "1.4.0",
"sinon": "^2.3.5",
"typescript": "2.9.1-insiders.20180516",
"typescript": "3.0.1",
"vscode-chrome-debug-core": "^3.23.8",
"zone.js": "^0.7.3"
},
Expand Down
4 changes: 0 additions & 4 deletions tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@
"diagnostics": true
},
"include": [
// TODO(bckenny): unnecessary workaround until https://github.com/Microsoft/TypeScript/issues/24062
"node_modules/@types/node/index.d.ts",
"third-party/devtools/*.js",

"lighthouse-cli/**/*.js",
"lighthouse-core/**/*.js",
"lighthouse-extension/app/src/*.js",
Expand Down
6 changes: 3 additions & 3 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -6199,9 +6199,9 @@ typedarray@^0.0.6:
version "0.0.6"
resolved "https://registry.yarnpkg.com/typedarray/-/typedarray-0.0.6.tgz#867ac74e3864187b1d3d47d996a78ec5c8830777"

typescript@2.9.1-insiders.20180516:
version "2.9.1-insiders.20180516"
resolved "https://registry.yarnpkg.com/typescript/-/typescript-2.9.1-insiders.20180516.tgz#aab5261edb2c162c2d0c1754bb3092d4ff6efed0"
typescript@3.0.1:
version "3.0.1"
resolved "https://registry.yarnpkg.com/typescript/-/typescript-3.0.1.tgz#43738f29585d3a87575520a4b93ab6026ef11fdb"

uglify-js@^2.6:
version "2.7.3"
Expand Down