Repository: GoogleChrome/lighthouse
Pull request #9832 (2019-10-12 02:35:05)

core: rename Element to LHElement
While checking out the ts 3.7 beta, this typing mistake became apparent. We use the global `Element` type declaration (dom lib) where we really want our `lib/element.js`. Renaming to `LHElement` is a simple solution to this name clash.

![image](https://user-images.githubusercontent.com/4071474/66693428-8476c280-ec5d-11e9-93d3-cc3ffba92ac8.png)

the error was uncovered likely thanks to changes required for this bugfix: https://devblogs.microsoft.com/typescript/announcing-typescript-3-7-beta/ "Local and Imported Type Declarations Now Conflict"


Patch:
diff --git a/build/build-bundle.js b/build/build-bundle.js
index a05679995..f1f32a3de 100644
--- a/build/build-bundle.js
+++ b/build/build-bundle.js
@@ -37,6 +37,8 @@ const locales = fs.readdirSync(__dirname + '/../lighthouse-core/lib/i18n/locales
 const isDevtools = file => path.basename(file).includes('devtools');
 /** @param {string} file */
 const isExtension = file => path.basename(file).includes('extension');
+/** @param {string} file */
+const isLightrider = file => path.basename(file).includes('lightrider');
 
 const BANNER = `// lighthouse, browserified. ${VERSION} (${COMMIT_HASH})\n`;
 const DEBUG = false; // true for sourcemaps
@@ -71,8 +73,9 @@ async function browserifyFile(entryPath, distPath) {
   // Don't include the desktop protocol connection.
   bundle.ignore(require.resolve('../lighthouse-core/gather/connections/cri.js'));
 
-  // Dont include the stringified report in DevTools.
-  if (isDevtools(entryPath)) {
+  // Don't include the stringified report in DevTools - see devtools-report-assets.js
+  // Don't include in Lightrider - HTML generation isn't supported, so report assets aren't needed.
+  if (isDevtools(entryPath) || isLightrider(entryPath)) {
     bundle.ignore(require.resolve('../lighthouse-core/report/html/html-report-assets.js'));
   }
 
diff --git a/build/build-lightrider-bundles.js b/build/build-lightrider-bundles.js
index 08cee17d7..0fa8c562a 100644
--- a/build/build-lightrider-bundles.js
+++ b/build/build-lightrider-bundles.js
@@ -10,6 +10,7 @@ const fs = require('fs');
 const path = require('path');
 const makeDir = require('make-dir');
 const bundleBuilder = require('./build-bundle.js');
+const {minifyFileTransform} = require('./build-utils.js');
 
 const distDir = path.join(__dirname, '..', 'dist', 'lightrider');
 const sourceDir = __dirname + '/../clients/lightrider';
@@ -37,7 +38,11 @@ function buildEntryPoint() {
 function buildReportGenerator() {
   browserify(generatorFilename, {standalone: 'ReportGenerator'})
     // Transform the fs.readFile etc into inline strings.
-    .transform('@wardpeet/brfs', {global: true, parserOpts: {ecmaVersion: 10}})
+    .transform('@wardpeet/brfs', {
+      readFileSyncTransform: minifyFileTransform,
+      global: true,
+      parserOpts: {ecmaVersion: 10},
+    })
     .bundle((err, src) => {
       if (err) throw err;
       fs.writeFileSync(bundleOutFile, src.toString());
diff --git a/build/build-utils.js b/build/build-utils.js
new file mode 100644
index 000000000..a8a4423b7
--- /dev/null
+++ b/build/build-utils.js
@@ -0,0 +1,37 @@
+/**
+ * @license Copyright 2019 Google Inc. All Rights Reserved.
+ * 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.
+ */
+'use strict';
+
+const stream = require('stream');
+const terser = require('terser');
+
+/**
+ * Minifies file which are read by fs.readFileSync (brfs)
+ *
+ * @param {string} file
+ */
+function minifyFileTransform(file) {
+  return new stream.Transform({
+    transform(chunk, enc, next) {
+      if (file.endsWith('.js')) {
+        const result = terser.minify(chunk.toString());
+        if (result.error) {
+          throw result.error;
+        }
+
+        this.push(result.code);
+      } else {
+        this.push(chunk);
+      }
+
+      next();
+    },
+  });
+}
+
+module.exports = {
+  minifyFileTransform,
+};
diff --git a/build/build-viewer.js b/build/build-viewer.js
index b8b490095..c3db40f4b 100644
--- a/build/build-viewer.js
+++ b/build/build-viewer.js
@@ -10,7 +10,6 @@ const path = require('path');
 const {promisify} = require('util');
 const readFileAsync = promisify(fs.readFile);
 const writeFileAsync = promisify(fs.writeFile);
-const stream = require('stream');
 
 const browserify = require('browserify');
 const cpy = require('cpy');
@@ -20,6 +19,7 @@ const lighthousePackage = require('../package.json');
 const makeDir = require('make-dir');
 const rimraf = require('rimraf');
 const terser = require('terser');
+const {minifyFileTransform} = require('./build-utils.js');
 
 const htmlReportAssets = require('../lighthouse-core/report/html/html-report-assets.js');
 const sourceDir = `${__dirname}/../lighthouse-viewer`;
@@ -99,30 +99,6 @@ async function html() {
   await safeWriteFileAsync(`${distDir}/index.html`, htmlSrc);
 }
 
-/**
- * Minifies file which are read by fs.readFileSync (brfs)
- *
- * @param {string} file
- */
-function minifyReadFileContent(file) {
-  return new stream.Transform({
-    transform(chunk, enc, next) {
-      if (file.endsWith('.js')) {
-        const result = terser.minify(chunk.toString());
-        if (result.error) {
-          throw result.error;
-        }
-
-        this.push(result.code);
-      } else {
-        this.push(chunk);
-      }
-
-      next();
-    },
-  });
-}
-
 /**
  * Combine multiple JS files into single viewer.js file.
  * @return {Promise<void>}
@@ -132,7 +108,7 @@ async function compileJs() {
   const generatorFilename = `${sourceDir}/../lighthouse-core/report/report-generator.js`;
   const generatorBrowserify = browserify(generatorFilename, {standalone: 'ReportGenerator'})
     .transform('@wardpeet/brfs', {
-      readFileSyncTransform: minifyReadFileContent,
+      readFileSyncTransform: minifyFileTransform,
     });
 
   /** @type {Promise<string>} */
diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js
index 7512c4570..bc3600037 100644
--- a/lighthouse-core/gather/driver.js
+++ b/lighthouse-core/gather/driver.js
@@ -7,7 +7,7 @@
 
 const NetworkRecorder = require('../lib/network-recorder.js');
 const emulation = require('../lib/emulation.js');
-const Element = require('../lib/element.js');
+const LHElement = require('../lib/lh-element.js');
 const LHError = require('../lib/lh-error.js');
 const NetworkRequest = require('../lib/network-request.js');
 const EventEmitter = require('events').EventEmitter;
@@ -528,8 +528,12 @@ class Driver {
     this.setNextProtocolTimeout(timeout);
     const response = await this.sendCommand('Runtime.evaluate', evaluationParams);
     if (response.exceptionDetails) {
-      // An error occurred before we could even create a Promise, should be *very* rare
-      return Promise.reject(new Error(`Evaluation exception: ${response.exceptionDetails.text}`));
+      // An error occurred before we could even create a Promise, should be *very* rare.
+      // Also occurs when the expression is not valid JavaScript.
+      const errorMessage = response.exceptionDetails.exception ?
+        response.exceptionDetails.exception.description :
+        response.exceptionDetails.text;
+      return Promise.reject(new Error(`Evaluation exception: ${errorMessage}`));
     }
     // Protocol should always return a 'result' object, but it is sometimes undefined.  See #6026.
     if (response.result === undefined) {
@@ -1224,7 +1228,7 @@ class Driver {
 
   /**
    * @param {string} selector Selector to find in the DOM
-   * @return {Promise<Element|null>} The found element, or null, resolved in a promise
+   * @return {Promise<LHElement|null>} The found element, or null, resolved in a promise
    */
   async querySelector(selector) {
     const documentResponse = await this.sendCommand('DOM.getDocument');
@@ -1238,12 +1242,12 @@ class Driver {
     if (targetNode.nodeId === 0) {
       return null;
     }
-    return new Element(targetNode, this);
+    return new LHElement(targetNode, this);
   }
 
   /**
    * @param {string} selector Selector to find in the DOM
-   * @return {Promise<Array<Element>>} The found elements, or [], resolved in a promise
+   * @return {Promise<Array<LHElement>>} The found elements, or [], resolved in a promise
    */
   async querySelectorAll(selector) {
     const documentResponse = await this.sendCommand('DOM.getDocument');
@@ -1254,11 +1258,11 @@ class Driver {
       selector,
     });
 
-    /** @type {Array<Element>} */
+    /** @type {Array<LHElement>} */
     const elementList = [];
     targetNodeList.nodeIds.forEach(nodeId => {
       if (nodeId !== 0) {
-        elementList.push(new Element({nodeId}, this));
+        elementList.push(new LHElement({nodeId}, this));
       }
     });
     return elementList;
@@ -1268,13 +1272,13 @@ class Driver {
    * Returns the flattened list of all DOM elements within the document.
    * @param {boolean=} pierce Whether to pierce through shadow trees and iframes.
    *     True by default.
-   * @return {Promise<Array<Element>>} The found elements, or [], resolved in a promise
+   * @return {Promise<Array<LHElement>>} The found elements, or [], resolved in a promise
    */
   getElementsInDocument(pierce = true) {
     return this.getNodesInDocument(pierce)
       .then(nodes => nodes
         .filter(node => node.nodeType === 1)
-        .map(node => new Element({nodeId: node.nodeId}, this))
+        .map(node => new LHElement({nodeId: node.nodeId}, this))
       );
   }
 
diff --git a/lighthouse-core/lib/element.js b/lighthouse-core/lib/lh-element.js
similarity index 97%
rename from lighthouse-core/lib/element.js
rename to lighthouse-core/lib/lh-element.js
index e17a5e9fc..5b9d34c01 100644
--- a/lighthouse-core/lib/element.js
+++ b/lighthouse-core/lib/lh-element.js
@@ -7,7 +7,7 @@
 
 const Driver = require('../gather/driver.js'); // eslint-disable-line no-unused-vars
 
-class Element {
+class LHElement {
   /**
    * @param {{nodeId: number}} element
    * @param {Driver} driver
@@ -68,4 +68,4 @@ class Element {
   }
 }
 
-module.exports = Element;
+module.exports = LHElement;
diff --git a/package.json b/package.json
index f628bb17e..9696d3bbb 100644
--- a/package.json
+++ b/package.json
@@ -187,11 +187,15 @@
     },
     {
       "path": "./dist/viewer/src/viewer.js",
-      "maxSize": "76 kB"
+      "maxSize": "65 kB"
     },
     {
       "path": "./dist/lighthouse-dt-bundle.js",
       "maxSize": "400 kB"
+    },
+    {
+      "path": "./dist/lightrider/report-generator-bundle.js",
+      "maxSize": "50 kB"
     }
   ],
   "nyc": {


Test patch:
diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js
index d78ba2c97..8b87bf6f2 100644
--- a/lighthouse-core/test/gather/driver-test.js
+++ b/lighthouse-core/test/gather/driver-test.js
@@ -7,7 +7,7 @@
 
 const Driver = require('../../gather/driver.js');
 const Connection = require('../../gather/connections/connection.js');
-const Element = require('../../lib/element.js');
+const LHElement = require('../../lib/lh-element.js');
 const EventEmitter = require('events').EventEmitter;
 const {protocolGetVersionResponse} = require('./fake-driver.js');
 const {createMockSendCommandFn, createMockOnceFn} = require('./mock-commands.js');
@@ -108,7 +108,7 @@ describe('.querySelector(All)', () => {
       .mockResponse('DOM.querySelector', {nodeId: 231});
 
     const result = await driver.querySelector('meta head');
-    expect(result).toBeInstanceOf(Element);
+    expect(result).toBeInstanceOf(LHElement);
   });
 
   it('returns [] when DOM.querySelectorAll finds no node', async () => {
@@ -127,7 +127,7 @@ describe('.querySelector(All)', () => {
 
     const result = await driver.querySelectorAll('#no.matches');
     expect(result).toHaveLength(1);
-    expect(result[0]).toBeInstanceOf(Element);
+    expect(result[0]).toBeInstanceOf(LHElement);
   });
 });
 
diff --git a/lighthouse-core/test/lib/element-test.js b/lighthouse-core/test/lib/lh-element-test.js
similarity index 82%
rename from lighthouse-core/test/lib/element-test.js
rename to lighthouse-core/test/lib/lh-element-test.js
index c97f66eb9..b2f204283 100644
--- a/lighthouse-core/test/lib/element-test.js
+++ b/lighthouse-core/test/lib/lh-element-test.js
@@ -5,7 +5,7 @@
  */
 'use strict';
 
-const Element = require('../../lib/element.js');
+const LHElement = require('../../lib/lh-element.js');
 const assert = require('assert');
 
 class DriverStub {
@@ -28,7 +28,7 @@ class DriverStub {
 }
 
 /* global describe, it, beforeEach */
-describe('Element', () => {
+describe('LHElement', () => {
   let stubbedDriver;
   let stubbedElement;
 
@@ -39,38 +39,38 @@ describe('Element', () => {
 
   it('throws when no driver or element is passed', () => {
     assert.throws(() => {
-      const _ = new Element();
+      const _ = new LHElement();
     });
   });
 
   it('throws when no driver is passed', () => {
     assert.throws(() => {
-      const _ = new Element(stubbedElement, undefined);
+      const _ = new LHElement(stubbedElement, undefined);
     });
   });
 
   it('throws when no element is passed', () => {
     assert.throws(() => {
-      const _ = new Element(undefined, stubbedDriver);
+      const _ = new LHElement(undefined, stubbedDriver);
     });
   });
 
   it('returns null from getAttribute when no attribute found', () => {
-    const element = new Element(stubbedElement, stubbedDriver);
+    const element = new LHElement(stubbedElement, stubbedDriver);
     return element.getAttribute('notanattribute').then(value => {
       assert.equal(value, null);
     });
   });
 
   it('returns attribute value from getAttribute', () => {
-    const element = new Element(stubbedElement, stubbedDriver);
+    const element = new LHElement(stubbedElement, stubbedDriver);
     return element.getAttribute('rel').then(value => {
       assert.equal(value, 'manifest');
     });
   });
 
   it('returns property value from getProperty', () => {
-    const element = new Element(stubbedElement, stubbedDriver);
+    const element = new LHElement(stubbedElement, stubbedDriver);
     return element.getProperty('test').then(value => {
       assert.equal(value, '123');
     });