chore: cleanup puppeteer.connect({browserURL}) (#3766)

This patch: - renames `browserUrl` into `browserURL` - cleans up some code - adds tests for error handling References #3537
puppeteer · Jan 14, 2019 · d346cb5 · d346cb5
1 parent 15af75f
commit d346cb5
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 42 deletions.
diff --git a/docs/api.md b/docs/api.md
@@ -445,7 +445,7 @@ puppeteer.launch().then(async browser => {
 #### puppeteer.connect(options)
 - `options` <[Object]>
   - `browserWSEndpoint` <?[string]> a [browser websocket endpoint](#browserwsendpoint) to connect to.
-  - `browserUrl` <?[string]> a browser url to connect to, in format `http://${host}:${port}`. Use interchangeably with `browserWSEndpoint` to let Puppeteer fetch it from [metadata endpoint](https://chromedevtools.github.io/devtools-protocol/#how-do-i-access-the-browser-target).
+  - `browserURL` <?[string]> a browser url to connect to, in format `http://${host}:${port}`. Use interchangeably with `browserWSEndpoint` to let Puppeteer fetch it from [metadata endpoint](https://chromedevtools.github.io/devtools-protocol/#how-do-i-access-the-browser-target).
   - `ignoreHTTPSErrors` <[boolean]> Whether to ignore HTTPS errors during navigation. Defaults to `false`.
   - `defaultViewport` <?[Object]> Sets a consistent viewport for each page. Defaults to an 800x600 viewport. `null` disables the default viewport.
     - `width` <[number]> page width in pixels.

diff --git a/lib/Launcher.js b/lib/Launcher.js
@@ -24,7 +24,7 @@ const {Connection} = require('./Connection');
 const {Browser} = require('./Browser');
 const readline = require('readline');
 const fs = require('fs');
-const {helper, debugError} = require('./helper');
+const {helper, assert, debugError} = require('./helper');
 const {TimeoutError} = require('./Errors');
 const WebSocketTransport = require('./WebSocketTransport');
 const PipeTransport = require('./PipeTransport');
@@ -278,33 +278,33 @@ class Launcher {
   }
 
   /**
-   * @param {!(Launcher.BrowserOptions & {browserWSEndpoint?: string, browserUrl?: string, transport?: !Puppeteer.ConnectionTransport})} options
+   * @param {!(Launcher.BrowserOptions & {browserWSEndpoint?: string, browserURL?: string, transport?: !Puppeteer.ConnectionTransport})} options
    * @return {!Promise<!Browser>}
    */
   async connect(options) {
     const {
       browserWSEndpoint,
-      browserUrl,
+      browserURL,
       ignoreHTTPSErrors = false,
       defaultViewport = {width: 800, height: 600},
       transport,
       slowMo = 0,
     } = options;
 
-    let connectionUrl;
-    let connectionTransport;
+    assert(Number(!!browserWSEndpoint) + Number(!!browserURL) + Number(!!transport) === 1, 'Exactly one of browserWSEndpoint, browserURL or transport must be passed to puppeteer.connect');
 
-    if (browserWSEndpoint)
-      connectionUrl = browserWSEndpoint;
-    else if (!browserWSEndpoint && browserUrl)
-      connectionUrl = await getWSEndpoint(browserUrl);
-
-    if (transport)
-      connectionTransport = transport;
-    else
-      connectionTransport = await WebSocketTransport.create(connectionUrl);
+    let connection = null;
+    if (transport) {
+      connection = new Connection('', transport, slowMo);
+    } else if (browserWSEndpoint) {
+      const connectionTransport = await WebSocketTransport.create(browserWSEndpoint);
+      connection = new Connection(browserWSEndpoint, connectionTransport, slowMo);
+    } else if (browserURL) {
+      const connectionURL = await getWSEndpoint(browserURL);
+      const connectionTransport = await WebSocketTransport.create(connectionURL);
+      connection = new Connection(connectionURL, connectionTransport, slowMo);
+    }
 
-    const connection = new Connection(connectionUrl, connectionTransport, slowMo);
     const {browserContextIds} = await connection.send('Target.getBrowserContexts');
     return Browser.create(connection, browserContextIds, ignoreHTTPSErrors, defaultViewport, null, () => connection.send('Browser.close').catch(debugError));
   }
@@ -393,45 +393,35 @@ function waitForWSEndpoint(chromeProcess, timeout, preferredRevision) {
 }
 
 /**
- * @param {string} browserUrl
+ * @param {string} browserURL
  * @return {!Promise<string>}
  */
-function getWSEndpoint(browserUrl) {
+function getWSEndpoint(browserURL) {
   let resolve, reject;
-  const endpointUrl = URL.resolve(browserUrl, '/json/version');
-  const requestOptions = Object.assign(URL.parse(endpointUrl), { method: 'GET' });
   const promise = new Promise((res, rej) => { resolve = res; reject = rej; });
 
-  function handleRequestEnd(data) {
-    try {
-      const {webSocketDebuggerUrl} = JSON.parse(data);
-      resolve(webSocketDebuggerUrl);
-    } catch (e) {
-      handleRequestError(e);
-    }
-  }
-
-  function handleRequestError(err) {
-    reject(new Error(`Failed to fetch browser webSocket url from ${endpointUrl}: ${err}`));
-  }
-
+  const endpointURL = URL.resolve(browserURL, '/json/version');
+  const requestOptions = Object.assign(URL.parse(endpointURL), { method: 'GET' });
   const request = http.request(requestOptions, res => {
     let data = '';
     if (res.statusCode !== 200) {
-      // consume response data to free up memory
+      // Consume response data to free up memory.
       res.resume();
-      handleRequestError(res.statusCode);
+      reject(new Error('HTTP ' + res.statusCode));
       return;
     }
     res.setEncoding('utf8');
     res.on('data', chunk => data += chunk);
-    res.on('end', () => handleRequestEnd(data));
+    res.on('end', () => resolve(JSON.parse(data).webSocketDebuggerUrl));
   });
 
-  request.on('error', handleRequestError);
+  request.on('error', reject);
   request.end();
 
-  return promise;
+  return promise.catch(e => {
+    e.message = `Failed to fetch browser webSocket url from ${endpointURL}: ` + e.message;
+    throw e;
+  });
 }
 
 /**

diff --git a/lib/Puppeteer.js b/lib/Puppeteer.js
@@ -37,7 +37,7 @@ module.exports = class {
   }
 
   /**
-   * @param {!(Launcher.BrowserOptions & {browserWSEndpoint?: string, browserUrl?: string, transport?: !Puppeteer.ConnectionTransport})} options
+   * @param {!(Launcher.BrowserOptions & {browserWSEndpoint?: string, browserURL?: string, transport?: !Puppeteer.ConnectionTransport})} options
    * @return {!Promise<!Puppeteer.Browser>}
    */
   connect(options) {

diff --git a/test/puppeteer.spec.js b/test/puppeteer.spec.js
@@ -348,17 +348,40 @@ module.exports.addTests = function({testRunner, expect, defaultBrowserOptions})
         const originalBrowser = await puppeteer.launch(Object.assign({}, defaultBrowserOptions, {
           args: ['--remote-debugging-port=21222']
         }));
-        const browserUrl = 'http://127.0.0.1:21222';
+        const browserURL = 'http://127.0.0.1:21222';
 
-        const browser1 = await puppeteer.connect({browserUrl});
+        const browser1 = await puppeteer.connect({browserURL});
         const page1 = await browser1.newPage();
         expect(await page1.evaluate(() => 7 * 8)).toBe(56);
         browser1.disconnect();
 
-        const browser2 = await puppeteer.connect({browserUrl: browserUrl + '/'});
+        const browser2 = await puppeteer.connect({browserURL: browserURL + '/'});
         const page2 = await browser2.newPage();
         expect(await page2.evaluate(() => 8 * 7)).toBe(56);
         browser2.disconnect();
+        originalBrowser.close();
+      });
+      it('should throw when using both browserWSEndpoint and browserURL', async({server}) => {
+        const originalBrowser = await puppeteer.launch(Object.assign({}, defaultBrowserOptions, {
+          args: ['--remote-debugging-port=21222']
+        }));
+        const browserURL = 'http://127.0.0.1:21222';
+
+        let error = null;
+        await puppeteer.connect({browserURL, browserWSEndpoint: originalBrowser.wsEndpoint()}).catch(e => error = e);
+        expect(error.message).toContain('Exactly one of browserWSEndpoint, browserURL or transport');
+
+        originalBrowser.close();
+      });
+      it('should throw when trying to connect to non-existing browser', async({server}) => {
+        const originalBrowser = await puppeteer.launch(Object.assign({}, defaultBrowserOptions, {
+          args: ['--remote-debugging-port=21222']
+        }));
+        const browserURL = 'http://127.0.0.1:32333';
+
+        let error = null;
+        await puppeteer.connect({browserURL}).catch(e => error = e);
+        expect(error.message).toContain('Failed to fetch browser webSocket url from');
 
         originalBrowser.close();
       });