From 3d7ae2a2591a55e4f94c67d62a22f7103698cba5 Mon Sep 17 00:00:00 2001 From: Joel Einbinder Date: Fri, 24 Aug 2018 15:17:36 -0700 Subject: [PATCH] fix: fix null-type bugs (#3137) I ran TypeScript against our code with `strictNullChecks` on. Most of the errors generated are noise, because TypeScript doesn't understand how our `assert` method works. But some were legitimate bugs. They are fixed in this patch. --- lib/Browser.js | 2 +- lib/Connection.js | 6 ++++-- lib/ExecutionContext.js | 2 +- lib/FrameManager.js | 7 ++++--- lib/Input.js | 2 +- lib/Launcher.js | 2 +- lib/NetworkManager.js | 4 ++-- lib/Page.js | 1 + lib/Target.js | 2 +- 9 files changed, 16 insertions(+), 12 deletions(-) diff --git a/lib/Browser.js b/lib/Browser.js index 8ef68c6adb3..61531516cc7 100644 --- a/lib/Browser.js +++ b/lib/Browser.js @@ -161,7 +161,7 @@ class Browser extends EventEmitter { } /** - * @param {string} contextId + * @param {?string} contextId * @return {!Promise} */ async _createPageInContext(contextId) { diff --git a/lib/Connection.js b/lib/Connection.js index 1670a9ee70e..a70d9d98e6a 100644 --- a/lib/Connection.js +++ b/lib/Connection.js @@ -172,6 +172,7 @@ class CDPSession extends EventEmitter { this._lastId = 0; /** @type {!Map}*/ this._callbacks = new Map(); + /** @type {null|Connection|CDPSession} */ this._connection = connection; this._targetType = targetType; this._sessionId = sessionId; @@ -234,6 +235,8 @@ class CDPSession extends EventEmitter { } async detach() { + if (!this._connection) + throw new Error(`Session already detached. Most likely the ${this._targetType} has been closed.`); await this._connection.send('Target.detachFromTarget', {sessionId: this._sessionId}); } @@ -266,8 +269,7 @@ function createProtocolError(error, method, object) { let message = `Protocol error (${method}): ${object.error.message}`; if ('data' in object.error) message += ` ${object.error.data}`; - if (object.error.message) - return rewriteError(error, message); + return rewriteError(error, message); } /** diff --git a/lib/ExecutionContext.js b/lib/ExecutionContext.js index ca6b0d2d1e3..c2460bc488a 100644 --- a/lib/ExecutionContext.js +++ b/lib/ExecutionContext.js @@ -473,7 +473,7 @@ class ElementHandle extends JSHandle { const viewport = this._page.viewport(); - if (boundingBox.width > viewport.width || boundingBox.height > viewport.height) { + if (viewport && (boundingBox.width > viewport.width || boundingBox.height > viewport.height)) { const newViewport = { width: Math.max(viewport.width, Math.ceil(boundingBox.width)), height: Math.max(viewport.height, Math.ceil(boundingBox.height)), diff --git a/lib/FrameManager.js b/lib/FrameManager.js index eb1e5a89d9b..0bbb3122a60 100644 --- a/lib/FrameManager.js +++ b/lib/FrameManager.js @@ -118,7 +118,6 @@ class FrameManager extends EventEmitter { /** * @param {string} frameId * @param {?string} parentFrameId - * @return {?Frame} */ _onFrameAttached(frameId, parentFrameId) { if (this._frames.has(frameId)) @@ -265,14 +264,16 @@ class Frame { this._parentFrame = parentFrame; this._url = ''; this._id = frameId; + this._detached = false; /** @type {?Promise} */ this._documentPromise = null; - /** @type {?Promise} */ - this._contextPromise = null; + /** @type {!Promise} */ + this._contextPromise; this._contextResolveCallback = null; this._setDefaultContext(null); + /** @type {!Set} */ this._waitTasks = new Set(); this._loaderId = ''; diff --git a/lib/Input.js b/lib/Input.js index b70212b4c93..53abf676699 100644 --- a/lib/Input.js +++ b/lib/Input.js @@ -38,7 +38,7 @@ class Keyboard { /** * @param {string} key - * @param {{text: string}=} options + * @param {{text?: string}=} options */ async down(key, options = { text: undefined }) { const description = this._keyDescriptionForString(key); diff --git a/lib/Launcher.js b/lib/Launcher.js index b735c017391..9c6a2c52a2b 100644 --- a/lib/Launcher.js +++ b/lib/Launcher.js @@ -258,7 +258,7 @@ class Launcher { } /** - * @param {!(BrowserOptions & {browserWSEndpoint: string})=} options + * @param {!(BrowserOptions & {browserWSEndpoint: string})} options * @return {!Promise} */ static async connect(options) { diff --git a/lib/NetworkManager.js b/lib/NetworkManager.js index 2054c3b7bb5..69cc685f291 100644 --- a/lib/NetworkManager.js +++ b/lib/NetworkManager.js @@ -256,7 +256,7 @@ class NetworkManager extends EventEmitter { if (!request) return; const response = new Response(this._client, request, event.response.status, event.response.headers, - event.response.fromDiskCache, event.response.fromServiceWorker, event.response.securityDetails); + !!event.response.fromDiskCache, !!event.response.fromServiceWorker, event.response.securityDetails); request._response = response; this.emit(NetworkManager.Events.Response, response); } @@ -353,7 +353,7 @@ class Request { } /** - * @return {string} + * @return {string|undefined} */ postData() { return this._postData; diff --git a/lib/Page.js b/lib/Page.js index e118327410e..888d64ed835 100644 --- a/lib/Page.js +++ b/lib/Page.js @@ -834,6 +834,7 @@ class Page extends EventEmitter { clip.scale = 1; if (options.fullPage) { + assert(this._viewport, 'fullPage screenshots do not work without first setting viewport.'); const metrics = await this._client.send('Page.getLayoutMetrics'); const width = Math.ceil(metrics.contentSize.width); const height = Math.ceil(metrics.contentSize.height); diff --git a/lib/Target.js b/lib/Target.js index c844d4ad8e0..cd2cab172b3 100644 --- a/lib/Target.js +++ b/lib/Target.js @@ -77,7 +77,7 @@ class Target { } /** - * @return {Puppeteer.Target} + * @return {?Puppeteer.Target} */ opener() { const { openerId } = this._targetInfo;