From 4826cf8cb2b5fa4457f5a691227bfae199f15785 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Wed, 19 Apr 2017 00:19:33 -0700 Subject: [PATCH] initial closure type checking for report v2 --- .../closure/closure-type-checking.js | 123 ++++++++++-------- .../closure/conformance_config.textproto | 18 +-- .../closure/third_party/commonjs.js | 25 ++++ .../report/v2/renderer/details-renderer.js | 41 +++--- lighthouse-core/report/v2/renderer/dom.js | 24 +++- .../report/v2/renderer/report-renderer.js | 35 ++--- .../v2/renderer/details-renderer-test.js | 39 ------ .../v2/renderer/report-renderer-test.js | 2 + package.json | 2 +- yarn.lock | 30 +---- 10 files changed, 165 insertions(+), 174 deletions(-) create mode 100644 lighthouse-core/closure/third_party/commonjs.js diff --git a/lighthouse-core/closure/closure-type-checking.js b/lighthouse-core/closure/closure-type-checking.js index f012aedea6ae..6f017338d595 100755 --- a/lighthouse-core/closure/closure-type-checking.js +++ b/lighthouse-core/closure/closure-type-checking.js @@ -22,67 +22,78 @@ const gulp = require('gulp'); const gutil = require('gulp-util'); const replace = require('gulp-replace'); +// Flags to generate additional debug information. +const PRINT_AST = false; +const PRINT_CODE = !PRINT_AST && false; +const OUTPUT_FILE = PRINT_AST ? '../closure-tree.txt' : 'closure-output.js'; + /* eslint-disable camelcase */ -gulp.task('js-compile', function() { +gulp.task('compile-report', () => { return gulp.src([ - 'closure/typedefs/*.js', - 'closure/third_party/*.js', - 'audits/**/*.js', - 'lib/event-helpers.js', - 'lib/icons.js', - 'lib/styles-helpers.js', - 'lib/url-shim.js', - 'aggregator/**/*.js' + // externs + 'closure/third_party/commonjs.js', + + 'report/v2/renderer/details-renderer.js', + 'report/v2/renderer/dom.js', + 'report/v2/renderer/report-renderer.js', ]) - // Hack to remove `require`s that Closure currently can't resolve. - .pipe(replace('require(\'../lib/web-inspector\').Color.parse;', - 'WebInspector.Color.parse;')) - .pipe(replace('require(\'../lib/traces/tracing-processor\');', '/** @type {?} */ (null);')) - .pipe(replace('require(\'../lib/traces/devtools-timeline-model\');', - 'DevtoolsTimelineModel')) - .pipe(replace('require(\'speedline\');', 'function(arg) {};')) - .pipe(replace(/require\('(\.\.\/)*report\/formatter'\);/g, '{};')) - // Replace any non-local import (e.g. not starting with .) with a dummy type. These are likely - // the built-in Node modules. But not always, so TODO(samthor): Fix this. - .pipe(replace(/require\(\'[^\.].*?\'\)/g, '/** @type {*} */ ({})')) + // Ignore `module.exports` and `self.ClassName = ClassName` statements. + .pipe(replace(/^\s\smodule\.exports = \w+;$/gm, ';')) + .pipe(replace(/^\s\sself\.(\w+) = \1;$/gm, ';')) + + .pipe(closureCompiler({ + compilation_level: 'SIMPLE', + // new_type_inf: true, + language_in: 'ECMASCRIPT6_STRICT', + language_out: 'ECMASCRIPT5_STRICT', + warning_level: process.env.CI ? 'QUIET' : 'VERBOSE', + jscomp_error: [ + 'checkTypes', + ], + jscomp_warning: [ + // https://github.com/google/closure-compiler/wiki/Warnings + 'accessControls', + 'checkRegExp', + 'const', + 'reportUnknownTypes', + 'missingProperties', + 'missingReturn', + 'strictModuleDepCheck', + 'typeInvalidation', + 'undefinedNames', + 'visibility', - .pipe(closureCompiler({ - compilation_level: 'SIMPLE', - process_common_js_modules: true, - new_type_inf: true, - checks_only: true, - language_in: 'ECMASCRIPT6_STRICT', - language_out: 'ECMASCRIPT5_STRICT', - warning_level: process.env.CI ? 'QUIET' : 'VERBOSE', - jscomp_error: [ - 'checkTypes', - 'conformanceViolations' - ], - jscomp_warning: [ - // https://github.com/google/closure-compiler/wiki/Warnings - 'accessControls', - 'checkRegExp', - 'const', - // 'reportUnknownTypes', - 'missingProperties', - 'missingReturn', - 'newCheckTypes', - 'strictModuleDepCheck', - 'typeInvalidation', - 'undefinedNames', - 'visibility' - ], - conformance_configs: 'closure/conformance_config.textproto' - })) - .on('error', error => { - gutil.log('Closure compilation failed. Check `closure-error.log` for details.'); - require('fs').writeFileSync('closure-error.log', error); - }) - .on('end', () => { - gutil.log('Closure compilation successful.'); - }); + 'checkDebuggerStatement', + 'externsValidation', + 'uselessCode', + 'ambiguousFunctionDecl', + 'checkTypes', + 'es3', + 'es5Strict', + 'globalThis', + 'nonStandardJsDocs', + 'suspiciousCode', + 'unknownDefines', + + // nullable/undefined checker when new_type_inf enabled. + 'newCheckTypesAllChecks', + ], + conformance_configs: 'closure/conformance_config.textproto', + + // Debug output control. + checks_only: !PRINT_CODE, + print_tree: PRINT_AST, + js_output_file: OUTPUT_FILE, + formatting: 'PRETTY_PRINT', + preserve_type_annotations: true, + })) + .pipe(gulp.dest('../')) + .on('end', () => { + gutil.log('Closure compilation successful.'); + }); }); + /* eslint-enable */ -gulp.start('js-compile'); +gulp.start('compile-report'); diff --git a/lighthouse-core/closure/conformance_config.textproto b/lighthouse-core/closure/conformance_config.textproto index 6b9c4929afac..06dfec3dae8a 100644 --- a/lighthouse-core/closure/conformance_config.textproto +++ b/lighthouse-core/closure/conformance_config.textproto @@ -15,19 +15,19 @@ # This file defines the additional JS conformance tests run over lighthouse code # when compiled with the Closure Compiler. For more, see https://github.com/google/closure-compiler/wiki/JS-Conformance-Framework +# Better covered by newCheckTypesAllChecks under NTI +# requirement: { +# type: CUSTOM +# java_class: 'com.google.javascript.jscomp.ConformanceRules$BanNullDeref' +# error_message: 'Dereferencing `null` or `undefined` is usually an error. See https://github.com/google/closure-compiler/wiki/JS-Conformance-Framework#bannullderef' +# } + requirement: { type: CUSTOM - java_class: 'com.google.javascript.jscomp.ConformanceRules$BanNullDeref' - error_message: 'Dereferencing `null` or `undefined` is usually an error. See https://github.com/google/closure-compiler/wiki/JS-Conformance-Framework#bannullderef' + java_class: 'com.google.javascript.jscomp.ConformanceRules$BanUnknownThis' + error_message: 'References to `this` that are typed as `unknown` are not allowed. See https://github.com/google/closure-compiler/wiki/JS-Conformance-Framework#banunknownthis' } -# TODO(bckenny): fix unknown this error(s) -# requirement: { -# type: CUSTOM -# java_class: 'com.google.javascript.jscomp.ConformanceRules$BanUnknownThis' -# error_message: 'References to `this` that are typed as `unknown` are not allowed. See https://github.com/google/closure-compiler/wiki/JS-Conformance-Framework#banunknownthis' -# } - requirement: { type: CUSTOM java_class: 'com.google.javascript.jscomp.ConformanceRules$BanUnknownTypedClassPropsReferences' diff --git a/lighthouse-core/closure/third_party/commonjs.js b/lighthouse-core/closure/third_party/commonjs.js new file mode 100644 index 000000000000..1e7ce291745c --- /dev/null +++ b/lighthouse-core/closure/third_party/commonjs.js @@ -0,0 +1,25 @@ +/** + * Copyright 2017 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. + */ + +/** + * @externs + */ + +/** @const */ +const module = {}; + +/** @type {*} */ +module.exports; diff --git a/lighthouse-core/report/v2/renderer/details-renderer.js b/lighthouse-core/report/v2/renderer/details-renderer.js index ebf15a4650d5..98c9375d17d5 100644 --- a/lighthouse-core/report/v2/renderer/details-renderer.js +++ b/lighthouse-core/report/v2/renderer/details-renderer.js @@ -22,23 +22,22 @@ class DetailsRenderer { * @param {!DOM} dom */ constructor(dom) { + /** @private {!DOM} */ this._dom = dom; } /** - * @param {(!DetailsRenderer.DetailsJSON|!DetailsRenderer.CardsDetailsJSON)} details + * @param {!DetailsRenderer.DetailsJSON} details * @return {!Element} */ render(details) { switch (details.type) { case 'text': return this._renderText(details); - case 'block': - return this._renderBlock(details); case 'cards': return this._renderCards(/** @type {!DetailsRenderer.CardsDetailsJSON} */ (details)); case 'list': - return this._renderList(details); + return this._renderList(/** @type {!DetailsRenderer.ListDetailsJSON} */ (details)); default: throw new Error(`Unknown type: ${details.type}`); } @@ -55,20 +54,7 @@ class DetailsRenderer { } /** - * @param {!DetailsRenderer.DetailsJSON} block - * @return {!Element} - */ - _renderBlock(block) { - const element = this._dom.createElement('div', 'lh-block'); - const items = block.items || []; - for (const item of items) { - element.appendChild(this.render(item)); - } - return element; - } - - /** - * @param {!DetailsRenderer.DetailsJSON} list + * @param {!DetailsRenderer.ListDetailsJSON} list * @return {!Element} */ _renderList(list) { @@ -80,8 +66,7 @@ class DetailsRenderer { } const itemsElem = this._dom.createElement('div', 'lh-list__items'); - const items = list.items || []; - for (const item of items) { + for (const item of list.items) { itemsElem.appendChild(this.render(item)); } element.appendChild(itemsElem); @@ -128,17 +113,23 @@ if (typeof module !== 'undefined' && module.exports) { /** * @typedef {{ * type: string, - * text: (string|undefined), - * header: (!DetailsRenderer.DetailsJSON|undefined), - * items: (!Array|undefined) + * text: (string|undefined) * }} */ DetailsRenderer.DetailsJSON; // eslint-disable-line no-unused-expressions +/** + * @typedef {{ + * type: string, + * header: ({text: string}|undefined), + * items: !Array<{type: string, text: (string|undefined)}> + * }} + */ +DetailsRenderer.ListDetailsJSON; // eslint-disable-line no-unused-expressions + /** @typedef {{ * type: string, - * text: string, - * header: !DetailsRenderer.DetailsJSON, + * header: ({text: string}|undefined), * items: !Array<{title: string, value: string, snippet: (string|undefined), target: string}> * }} */ diff --git a/lighthouse-core/report/v2/renderer/dom.js b/lighthouse-core/report/v2/renderer/dom.js index 0b3ea063a4bb..09efc6fb6dc8 100644 --- a/lighthouse-core/report/v2/renderer/dom.js +++ b/lighthouse-core/report/v2/renderer/dom.js @@ -22,6 +22,7 @@ class DOM { * @param {!Document} document */ constructor(document) { + /** @private {!Document} */ this._document = document; } @@ -33,9 +34,7 @@ class DOM { * set the attribute on the node. * @return {!Element} */ - createElement(name, className, attrs) { - // TODO(all): adopt `attrs` default arg when https://codereview.chromium.org/2821773002/ lands - attrs = attrs || {}; + createElement(name, className, attrs = {}) { const element = this._document.createElement(name); if (className) { element.className = className; @@ -56,7 +55,7 @@ class DOM { * @throws {Error} */ cloneTemplate(selector, context) { - const template = context.querySelector(selector); + const template = /** @type {HTMLTemplateElement} */ (context.querySelector(selector)); if (!template) { throw new Error(`Template not found: template${selector}`); } @@ -80,7 +79,7 @@ class DOM { // Append link if there are any. if (linkText && linkHref) { - const a = this.createElement('a'); + const a = /** @type {!HTMLAnchorElement} */ (this.createElement('a')); a.rel = 'noopener'; a.target = '_blank'; a.textContent = linkText; @@ -98,6 +97,21 @@ class DOM { document() { return this._document; } + + /** + * Guaranteed context.querySelector. Always returns an element or throws if + * nothing matches query. + * @param {!DocumentFragment|!Element} context + * @param {string} query + * @return {!Element} + */ + static find(context, query) { + const result = context.querySelector(query); + if (result === null) { + throw new Error(`query ${query} not found`); + } + return result; + } } if (typeof module !== 'undefined' && module.exports) { diff --git a/lighthouse-core/report/v2/renderer/report-renderer.js b/lighthouse-core/report/v2/renderer/report-renderer.js index 3079610c7743..8e3e56e63d18 100644 --- a/lighthouse-core/report/v2/renderer/report-renderer.js +++ b/lighthouse-core/report/v2/renderer/report-renderer.js @@ -22,7 +22,7 @@ * Dummy text for ensuring report robustness: pre$`post %%LIGHTHOUSE_JSON%% */ -/* globals self */ +/* globals self, DOM */ const RATINGS = { PASS: {label: 'pass', minScore: 75}, @@ -60,9 +60,11 @@ class ReportRenderer { * @param {!DetailsRenderer} detailsRenderer */ constructor(dom, detailsRenderer) { + /** @private {!DOM} */ this._dom = dom; + /** @private {!DetailsRenderer} */ this._detailsRenderer = detailsRenderer; - + /** @private {!Document|!Element} */ this._templateContext = this._dom.document(); } @@ -73,7 +75,7 @@ class ReportRenderer { renderReport(report) { try { return this._renderReport(report); - } catch (e) { + } catch (/** @type {!Error} */ e) { return this._renderException(e); } } @@ -88,13 +90,13 @@ class ReportRenderer { */ _populateScore(element, score, scoringMode, title, description) { // Fill in the blanks. - const valueEl = element.querySelector('.lh-score__value'); + const valueEl = DOM.find(element, '.lh-score__value'); valueEl.textContent = formatNumber(score); valueEl.classList.add(`lh-score__value--${calculateRating(score)}`, - `lh-score__value--${scoringMode}`); + `lh-score__value--${scoringMode}`); - element.querySelector('.lh-score__title').textContent = title; - element.querySelector('.lh-score__description') + DOM.find(element, '.lh-score__title').textContent = title; + DOM.find(element, '.lh-score__description') .appendChild(this._dom.createSpanFromMarkdown(description)); return /** @type {!Element} **/ (element); @@ -128,7 +130,7 @@ class ReportRenderer { } // Append audit details to header section so the entire audit is within a
. - const header = tmpl.querySelector('.lh-score__header'); + const header = /** @type {!HTMLDetailsElement} */ (DOM.find(tmpl, '.lh-score__header')); header.open = audit.score < 100; // expand failed audits if (audit.result.details) { header.appendChild(this._detailsRenderer.render(audit.result.details)); @@ -218,15 +220,17 @@ if (typeof module !== 'undefined' && module.exports) { /** * @typedef {{ - * id: string, weight: - * number, score: number, + * id: string, + * weight: number, + * score: number, * result: { * description: string, * displayValue: string, * helpText: string, * score: (number|boolean), * scoringMode: string, - * details: (!DetailsRenderer.DetailsJSON|!DetailsRenderer.CardsDetailsJSON|undefined) + * optimalValue: number, + * details: (!DetailsRenderer.DetailsJSON|undefined) * } * }} */ @@ -245,11 +249,10 @@ ReportRenderer.CategoryJSON; // eslint-disable-line no-unused-expressions /** * @typedef {{ - * lighthouseVersion: !string, - * generatedTime: !string, - * initialUrl: !string, - * url: !string, - * audits: ?Object, + * lighthouseVersion: string, + * generatedTime: string, + * initialUrl: string, + * url: string, * reportCategories: !Array * }} */ diff --git a/lighthouse-core/test/report/v2/renderer/details-renderer-test.js b/lighthouse-core/test/report/v2/renderer/details-renderer-test.js index e973e0ec735a..8eaaeb76666e 100644 --- a/lighthouse-core/test/report/v2/renderer/details-renderer-test.js +++ b/lighthouse-core/test/report/v2/renderer/details-renderer-test.js @@ -47,20 +47,6 @@ describe('DetailsRenderer', () => { assert.ok(el.classList.contains('lh-text'), 'adds classes'); }); - it('renders blocks', () => { - const el = renderer.render({ - type: 'block', - items: [ - {type: 'text', text: 'content 1'}, - {type: 'text', text: 'content 2'}, - ], - }); - - const children = el.querySelectorAll('.lh-text'); - assert.equal(children.length, 2, 'renders children'); - assert.ok(el.classList.contains('lh-block'), 'adds classes'); - }); - it('renders lists with headers', () => { const el = renderer.render({ type: 'list', @@ -95,31 +81,6 @@ describe('DetailsRenderer', () => { assert.equal(items.children.length, 3, 'did not render children'); }); - it('renders nested structures', () => { - const el = renderer.render({ - type: 'block', - items: [ - {type: 'text', text: 'content 1'}, - {type: 'text', text: 'content 2'}, - { - type: 'list', - header: {type: 'text', text: 'header'}, - items: [ - {type: 'text', text: 'sub-content 1'}, - {type: 'text', text: 'sub-content 2'}, - ] - }, - ], - }); - - const textChild = el.querySelector('.lh-block > .lh-text'); - const listChild = el.querySelector('.lh-block > .lh-details'); - const textSubChild = el.querySelector('.lh-block .lh-details .lh-text'); - assert.ok(textChild, 'did not render text children'); - assert.ok(listChild, 'did not render list child'); - assert.ok(textSubChild, 'did not render sub-children'); - }); - it('renders cards', () => { const list = { header: {type: 'text', text: 'View details'}, diff --git a/lighthouse-core/test/report/v2/renderer/report-renderer-test.js b/lighthouse-core/test/report/v2/renderer/report-renderer-test.js index bdc810f370f5..8b90a5908a25 100644 --- a/lighthouse-core/test/report/v2/renderer/report-renderer-test.js +++ b/lighthouse-core/test/report/v2/renderer/report-renderer-test.js @@ -33,6 +33,7 @@ describe('ReportRenderer V2', () => { before(() => { global.URL = URL; + global.DOM = DOM; const document = jsdom.jsdom(TEMPLATE_FILE); const dom = new DOM(document); const detailsRenderer = new DetailsRenderer(dom); @@ -41,6 +42,7 @@ describe('ReportRenderer V2', () => { after(() => { global.URL = undefined; + global.DOM = undefined; }); describe('renderReport', () => { diff --git a/package.json b/package.json index 3bbf15ed5c3f..3e6fa1e8b845 100644 --- a/package.json +++ b/package.json @@ -44,7 +44,7 @@ "coveralls": "^2.11.9", "eslint": "^3.12.0", "eslint-config-google": "^0.7.1", - "google-closure-compiler": "^20161201.0.0", + "google-closure-compiler": "20170409.0.0", "gulp": "^3.9.1", "gulp-concat": "^2.6.1", "gulp-declare": "^0.3.0", diff --git a/yarn.lock b/yarn.lock index 93dd709f3fe3..94fdb7a9c742 100644 --- a/yarn.lock +++ b/yarn.lock @@ -30,11 +30,7 @@ acorn@^3.0.4: version "3.3.0" resolved "https://registry.yarnpkg.com/acorn/-/acorn-3.3.0.tgz#45e37fb39e8da3f25baee3ff5369e2bb5f22017a" -acorn@^4.0.1: - version "4.0.3" - resolved "https://registry.yarnpkg.com/acorn/-/acorn-4.0.3.tgz#1a3e850b428e73ba6b09d1cc527f5aaad4d03ef1" - -acorn@^4.0.4: +acorn@^4.0.1, acorn@^4.0.4: version "4.0.11" resolved "https://registry.yarnpkg.com/acorn/-/acorn-4.0.11.tgz#edcda3bd937e7556410d42ed5860f67399c794c0" @@ -1241,12 +1237,12 @@ glogg@^1.0.0: dependencies: sparkles "^1.0.0" -google-closure-compiler@^20161201.0.0: - version "20161201.0.0" - resolved "https://registry.yarnpkg.com/google-closure-compiler/-/google-closure-compiler-20161201.0.0.tgz#2bcf0206060ad4f4031f64f73eaab7119ff76738" +google-closure-compiler@20170409.0.0: + version "20170409.0.0" + resolved "https://registry.yarnpkg.com/google-closure-compiler/-/google-closure-compiler-20170409.0.0.tgz#dc1be29a9f7eef8611364533b271b9fac757c970" dependencies: chalk "^1.0.0" - vinyl "^1.2.0" + vinyl "^2.0.1" vinyl-sourcemaps-apply "^0.2.0" got@^6.7.1: @@ -3053,16 +3049,12 @@ to-fast-properties@^1.0.1: version "1.0.2" resolved "https://registry.yarnpkg.com/to-fast-properties/-/to-fast-properties-1.0.2.tgz#f3f5c0c3ba7299a7ef99427e44633257ade43320" -tough-cookie@^2.3.2: +tough-cookie@^2.3.2, tough-cookie@~2.3.0: version "2.3.2" resolved "https://registry.yarnpkg.com/tough-cookie/-/tough-cookie-2.3.2.tgz#f081f76e4c85720e6c37a5faced737150d84072a" dependencies: punycode "^1.4.1" -tough-cookie@~2.3.0: - version "2.3.1" - resolved "https://registry.yarnpkg.com/tough-cookie/-/tough-cookie-2.3.1.tgz#99c77dfbb7d804249e8a299d4cb0fd81fef083fd" - tr46@~0.0.3: version "0.0.3" resolved "https://registry.yarnpkg.com/tr46/-/tr46-0.0.3.tgz#8184fd347dac9cdc185992f3a6622e14b9d9ab6a" @@ -3240,15 +3232,7 @@ vinyl@^0.5.0: clone-stats "^0.0.1" replace-ext "0.0.1" -vinyl@^1.2.0: - version "1.2.0" - resolved "https://registry.yarnpkg.com/vinyl/-/vinyl-1.2.0.tgz#5c88036cf565e5df05558bfc911f8656df218884" - dependencies: - clone "^1.0.0" - clone-stats "^0.0.1" - replace-ext "0.0.1" - -vinyl@^2.0.0: +vinyl@^2.0.0, vinyl@^2.0.1: version "2.0.1" resolved "https://registry.yarnpkg.com/vinyl/-/vinyl-2.0.1.tgz#1c3b4931e7ac4c1efee743f3b91a74c094407bb6" dependencies: