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

report: extract independent report types #12946

Merged
merged 3 commits into from
Aug 20, 2021
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
2 changes: 2 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
'use strict';

module.exports = {
// All subdirectory eslintrcs extend from this one.
root: true,
// start with google standard style
// https://github.com/google/eslint-config-google/blob/master/index.js
extends: ['eslint:recommended', 'google'],
Expand Down
1 change: 0 additions & 1 deletion clients/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
'use strict';

module.exports = {
extends: '../.eslintrc.js',
env: {
browser: true,
},
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/util-commonjs.js
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ class Util {
}

/**
* @param {LH.Config.Settings} settings
* @param {LH.Result['configSettings']} settings
* @return {!Array<{name: string, description: string}>}
*/
static getEnvironmentDisplayValues(settings) {
Expand All @@ -414,7 +414,7 @@ class Util {
}

/**
* @param {LH.Config.Settings} settings
* @param {LH.Result['configSettings']} settings
* @return {{deviceEmulation: string, networkThrottling: string, cpuThrottling: string}}
*/
static getEmulationDescriptions(settings) {
Expand Down
5 changes: 3 additions & 2 deletions lighthouse-treemap/types/treemap.d.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import {Logger as _Logger} from '../../report/renderer/logger.js';
import {FirebaseNamespace} from '@firebase/app-types';

// Import for needed DOM type augmentation.
import '../../report/types/augment-dom';

declare global {
class WebTreeMap {
constructor(data: any, options: WebTreeMapOptions);
Expand Down Expand Up @@ -45,5 +48,3 @@ declare global {
signal?: AbortSignal;
}
}

export {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the force module situation these days such that this isn't necessary? I feel like I remember reading a PR description or comment about this recently, but couldn't find it quickly.

Copy link
Member Author

@brendankenny brendankenny Aug 20, 2021

Choose a reason for hiding this comment

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

what's the force module situation these days such that this isn't necessary? I feel like I remember reading a PR description or comment about this recently, but couldn't find it quickly.

it's kind of dumb: if there's any import or export statements, the file is a module (which is why we had the empty ones in there before when everything was in one or two files and we didn't have any imports), and if you want an explicit declare global {} block in there it has to be a module, otherwise it's ambient and everything is a global type.

Fortunately(?) we almost always have import statements these days, and the error message is a lot better, basically saying you can't declare global unless this file is a module.

I vaguely recall a few years back in the es modules transition wars the idea of using that same heuristic to automatically detect if a file was an .mjs vs .cjs file without a file extension/package.json type entry, and then everyone realized that would be super fragile and annoying and decided not to go that way. But here it is anyways :)

1 change: 0 additions & 1 deletion lighthouse-viewer/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
'use strict';

module.exports = {
extends: '../.eslintrc.js',
env: {
browser: true,
},
Expand Down
17 changes: 8 additions & 9 deletions lighthouse-viewer/types/viewer.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,13 @@
*/

import _ReportGenerator = require('../../report/generator/report-generator.js');
import {DOM as _DOM} from '../../report/renderer/dom.js';
import {ReportRenderer as _ReportRenderer} from '../../report/renderer/report-renderer.js';
import {ReportUIFeatures as _ReportUIFeatures} from '../../report/renderer/report-ui-features.js';
import {Logger as _Logger} from '../../report/renderer/logger.js';
import {TextEncoding as _TextEncoding} from '../../report/renderer/text-encoding.js';
import {LighthouseReportViewer as _LighthouseReportViewer} from '../app/src/lighthouse-report-viewer.js';
import 'google.analytics';
import {FirebaseNamespace} from '@firebase/app-types';
import '@firebase/auth-types';
import _ReportResult from '../../report/types/report-result';

// Import for needed DOM type augmentation.
import '../../report/types/augment-dom';

declare global {
var ReportGenerator: typeof _ReportGenerator;
Expand All @@ -29,7 +26,9 @@ declare global {
// Inserted by viewer build.
LH_CURRENT_VERSION: string;
}

// Expose global types in LH namespace.
module LH {
export import ReportResult = _ReportResult;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh interesting I wouldn't have expected export import to be needed since we're reassigning.

does this differ from export type ReportResult = _ReportResult; or because it's a module we can't do that and export import is the only valid way?

Copy link
Member Author

@brendankenny brendankenny Aug 20, 2021

Choose a reason for hiding this comment

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

Oh interesting I wouldn't have expected export import to be needed since we're reassigning.

does this differ from export type ReportResult = _ReportResult; or because it's a module we can't do that and export import is the only valid way?

yeah, you get the interface but not the module, so e.g. LH.ReportResult.AuditRef won't resolve

}
}

// empty export to keep file a module
export {}
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
"dogfood-lhci": "./lighthouse-core/scripts/dogfood-lhci.sh",
"timing-trace": "node lighthouse-core/scripts/generate-timing-trace.js",
"changelog": "conventional-changelog --config ./build/changelog-generator/index.js --infile changelog.md --same-file",
"type-check": "tsc -b . && tsc -p lighthouse-viewer/ && tsc -p lighthouse-treemap/ && tsc -p flow-report/",
"type-check": "tsc -b . && tsc -b report/ && tsc -p lighthouse-viewer/ && tsc -p lighthouse-treemap/ && tsc -p flow-report/",
"i18n:checks": "./lighthouse-core/scripts/i18n/assert-strings-collected.sh",
"i18n:collect-strings": "node lighthouse-core/scripts/i18n/collect-strings.js",
"update:lantern-baseline": "node lighthouse-core/scripts/lantern/update-baseline-lantern-values.js",
Expand Down
18 changes: 18 additions & 0 deletions report/.eslintrc.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
Copy link
Member Author

Choose a reason for hiding this comment

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

added a new eslintrc for report/ to make it env browser and remove the ad hoc /* global document window */ and /* eslint-env browser */s sprinkled throughout some of the files.

Also, turns out eslintrc files automatically extend any other eslintrc files up through to the root directory, though I added a root: true in the root file just to make it clear

* @license Copyright 2021 The Lighthouse Authors. 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';

/**
* eslint does not support ESM rc files, so this must be a .cjs file.
* @see https://eslint.org/docs/user-guide/configuring/configuration-files#configuration-file-formats
* @see https://github.com/eslint/eslint/issues/13481
*/

module.exports = {
env: {
browser: true,
},
};
2 changes: 0 additions & 2 deletions report/clients/psi.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ import {PerformanceCategoryRenderer} from '../renderer/performance-category-rend
import {ReportUIFeatures} from '../renderer/report-ui-features.js';
import {Util} from '../renderer/util.js';

/* global window */

/** @typedef {{scoreGaugeEl: Element, perfCategoryEl: Element, finalScreenshotDataUri: string|null, scoreScaleEl: Element, installFeatures: Function}} PrepareLabDataResult */

/**
Expand Down
2 changes: 1 addition & 1 deletion report/clients/standalone.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
* The renderer code is bundled and injected into the report HTML along with the JSON report.
*/

/* global document window ga */
/* global ga */

import {DOM} from '../renderer/dom.js';
import {Logger} from '../renderer/logger.js';
Expand Down
2 changes: 1 addition & 1 deletion report/renderer/details-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const URL_PREFIXES = ['http://', 'https://', 'data:'];
export class DetailsRenderer {
/**
* @param {DOM} dom
* @param {{fullPageScreenshot?: LH.Artifacts.FullPageScreenshot}} [options]
* @param {{fullPageScreenshot?: LH.Audit.Details.FullPageScreenshot}} [options]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this starts to illustrate how the boundaries are hard to see here (glancing at this without context, I wouldn't expect LH.Audit to be within the self-contained report context

Long-term do you plan to renamespace the report to LHReport or LH.Report. or similar?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this starts to illustrate how the boundaries are hard to see here (glancing at this without context, I wouldn't expect LH.Audit to be within the self-contained report context

Long-term do you plan to renamespace the report to LHReport or LH.Report. or similar?

Yeah, it's a really good question. I'm not sure. I defaulted to maintaining the familiar LH.* hierarchy, but in the report files, the LH.Audit namespace is empty except for Details:

module LH {
  module Audit {
     export import Details = AuditDetails;
  }
}

https://github.com/GoogleChrome/lighthouse/pull/12946/files#diff-2a34e64f4e5c4c358613feaff0fbc2da80d47238fb89ca37fdc07c96ce8e1c2cR22-R24

so seems doubly weird to maintain it? Not that useful in the report if considered in isolation, and if the goal is consistency across files, it's actually misleading because you might think e.g. LH.Audit.ScoreDisplayMode is going to be in there too.

So long term it may make sense to re work the names, but then it's asking everyone to keep track of multiple names for the same thing...

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe if microsoft/TypeScript#41825 is fixed, core could keep the global types and the other areas with fewer files and fewer type needs could just do

/** @typedef {import('../../lhr/audit-details')} AuditDetails */

export class DetailsRenderer {
  /**
   * @param {DOM} dom
   * @param {{fullPageScreenshot?: AuditDetails.FullPageScreenshot}} [options]
   */
  constructor(dom, options = {}) {
    this._dom = dom;
    this._fullPageScreenshot = options.fullPageScreenshot;
  }
}

Without that issue fixed, though, we'd be stuck with

/** @typedef {import('../../lhr/audit-details').FullPageScreenshot} FullPageScreenshot */
/** @typedef {import('../../lhr/audit-details').OpportunityColumnHeading} OpportunityColumnHeading */
/** @typedef {import('../../lhr/audit-details').TableColumnHeading} TableColumnHeading */
/** @typedef {import('../../lhr/audit-details').List} List */
/** @typedef {import('../../lhr/audit-details').NodeValue} NodeValue */
// ...

:(

*/
constructor(dom, options = {}) {
this._dom = dom;
Expand Down
16 changes: 8 additions & 8 deletions report/renderer/element-screenshot-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,21 @@
*/

/** @typedef {import('./dom.js').DOM} DOM */
/** @typedef {LH.Artifacts.Rect} Rect */
/** @typedef {LH.Audit.Details.Rect} Rect */
/** @typedef {{width: number, height: number}} Size */

/**
* @typedef InstallOverlayFeatureParams
* @property {DOM} dom
* @property {Element} reportEl
* @property {Element} overlayContainerEl
* @property {LH.Artifacts.FullPageScreenshot} fullPageScreenshot
* @property {LH.Audit.Details.FullPageScreenshot} fullPageScreenshot
*/

import {Util} from './util.js';

/**
* @param {LH.Artifacts.FullPageScreenshot['screenshot']} screenshot
* @param {LH.Audit.Details.FullPageScreenshot['screenshot']} screenshot
* @param {LH.Audit.Details.Rect} rect
* @return {boolean}
*/
Expand Down Expand Up @@ -98,7 +98,7 @@ export class ElementScreenshotRenderer {
* @param {DOM} dom
* @param {HTMLElement} maskEl
* @param {{left: number, top: number}} positionClip
* @param {LH.Artifacts.Rect} elementRect
* @param {Rect} elementRect
* @param {Size} elementPreviewSize
*/
static renderClipPathInScreenshot(dom, maskEl, positionClip, elementRect, elementPreviewSize) {
Expand Down Expand Up @@ -132,7 +132,7 @@ export class ElementScreenshotRenderer {
* Allows for multiple Lighthouse reports to be rendered on the page, each with their
* own full page screenshot.
* @param {HTMLElement} el
* @param {LH.Artifacts.FullPageScreenshot['screenshot']} screenshot
* @param {LH.Audit.Details.FullPageScreenshot['screenshot']} screenshot
*/
static installFullPageScreenshot(el, screenshot) {
el.style.setProperty('--element-screenshot-url', `url(${screenshot.data})`);
Expand Down Expand Up @@ -195,7 +195,7 @@ export class ElementScreenshotRenderer {
/**
* Given the size of the element in the screenshot and the total available size of our preview container,
* compute the factor by which we need to zoom out to view the entire element with context.
* @param {LH.Artifacts.Rect} elementRectSC
* @param {Rect} elementRectSC
* @param {Size} renderContainerSizeDC
* @return {number}
*/
Expand All @@ -214,8 +214,8 @@ export class ElementScreenshotRenderer {
* Used to render both the thumbnail preview in details tables and the full-page screenshot in the lightbox.
* Returns null if element rect is outside screenshot bounds.
* @param {DOM} dom
* @param {LH.Artifacts.FullPageScreenshot['screenshot']} screenshot
* @param {LH.Artifacts.Rect} elementRectSC Region of screenshot to highlight.
* @param {LH.Audit.Details.FullPageScreenshot['screenshot']} screenshot
* @param {Rect} elementRectSC Region of screenshot to highlight.
* @param {Size} maxRenderSizeDC e.g. maxThumbnailSize or maxLightboxSize.
* @return {Element|null}
*/
Expand Down
4 changes: 2 additions & 2 deletions report/renderer/logger.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export class Logger {
this.el.textContent = msg;
this.el.classList.add('show');
if (autoHide) {
this._id = setTimeout(_ => {
this._id = setTimeout(() => {
this.el.classList.remove('show');
}, 7000);
}
Expand All @@ -61,7 +61,7 @@ export class Logger {

// Rethrow to make sure it's auditable as an error, but in a setTimeout so page
// recovers gracefully and user can try loading a report again.
setTimeout(_ => {
setTimeout(() => {
throw new Error(msg);
}, 0);
}
Expand Down
4 changes: 1 addition & 3 deletions report/renderer/report-ui-features.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
*/
'use strict';

/* eslint-env browser */

/**
* @fileoverview Adds tools button, print, and other dynamic functionality to
* the report.
Expand Down Expand Up @@ -707,7 +705,7 @@ export class ReportUIFeatures {

// cleanup.
this._document.body.removeChild(a);
setTimeout(_ => URL.revokeObjectURL(a.href), 500);
setTimeout(() => URL.revokeObjectURL(a.href), 500);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion report/renderer/text-encoding.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
'use strict';

/* global btoa atob window CompressionStream Response */
/* global CompressionStream */

const btoa_ = typeof btoa !== 'undefined' ?
btoa :
Expand Down
4 changes: 2 additions & 2 deletions report/renderer/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ export class Util {
}

/**
* @param {LH.Config.Settings} settings
* @param {LH.Result['configSettings']} settings
* @return {!Array<{name: string, description: string}>}
*/
static getEnvironmentDisplayValues(settings) {
Expand All @@ -411,7 +411,7 @@ export class Util {
}

/**
* @param {LH.Config.Settings} settings
* @param {LH.Result['configSettings']} settings
* @return {{deviceEmulation: string, networkThrottling: string, cpuThrottling: string}}
*/
static getEmulationDescriptions(settings) {
Expand Down
2 changes: 0 additions & 2 deletions report/test-assets/faux-psi.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@

/** @fileoverview This file is a glorified call of prepareLabData. */

/* global document window */

/** @typedef {import('../clients/psi.js').PrepareLabDataResult} PrepareLabDataResult */

(async function __initPsiReports__() {
Expand Down
2 changes: 1 addition & 1 deletion report/test/renderer/category-renderer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
'use strict';

/* eslint-env jest, browser */
/* eslint-env jest */

import {strict as assert} from 'assert';

Expand Down
2 changes: 1 addition & 1 deletion report/test/renderer/performance-category-renderer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
'use strict';

/* eslint-env jest, browser */
/* eslint-env jest */

import {strict as assert} from 'assert';

Expand Down
2 changes: 1 addition & 1 deletion report/test/renderer/pwa-category-renderer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
'use strict';

/* eslint-env jest, browser */
/* eslint-env jest */

import {strict as assert} from 'assert';

Expand Down
39 changes: 39 additions & 0 deletions report/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
{
"compilerOptions": {
"composite": true,
"outDir": "../.tmp/tsbuildinfo/report",
"emitDeclarationOnly": true,
"declarationMap": true,

// Limit to base JS and DOM defs.
"lib": ["es2020", "dom", "dom.iterable"],
// Don't include any node_module types.
"types": [],
"target": "es2020",
"module": "es2020",
"moduleResolution": "node",

"allowJs": true,
"checkJs": true,
"strict": true,
// TODO: remove the next line to be fully `strict`.
"useUnknownInCatchVariables": false,

// "listFiles": true,
// "noErrorTruncation": true,
"extendedDiagnostics": true,
},
"include": [
"**/*.js",
"types/**/*.d.ts",
],
"references": [
{"path": "../types/lhr/"},
{"path": "./generator/"}
],
"exclude": [
"generator/**/*.js",
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't realize generator/* files would be included even though generator/ is in references and has its own tsconfig. Having to explicitly exclude these is one more argument not to nest generator under report, I think (see other options in #12940 (comment)).

// These test files require further changes before they can be type checked.
"test/**/*.js",
],
}
24 changes: 24 additions & 0 deletions report/types/augment-dom.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/**
* @license Copyright 2021 The Lighthouse Authors. 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.
*/

/**
* @fileoverview Augment global scope with needed DOM APIs that are newer or not
* widely supported enough to be in tsc's lib `dom`.
*/

// Import to augment querySelector/querySelectorAll with stricter type checking.
import '../../types/query-selector';

declare global {
var CompressionStream: {
prototype: CompressionStream,
new (format: string): CompressionStream,
};

interface CompressionStream extends GenericTransformStream {
readonly format: string;
}
}
19 changes: 19 additions & 0 deletions report/types/buffer.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/**
* @license Copyright 2021 The Lighthouse Authors. 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.
*/

/**
* A minimal type for Node's `Buffer` to avoid importing all Node types for
* atob/btoa fallback for testing `text-encoding.js`.
*/

declare global {
class Buffer {
static from(str: string, encoding?: string): Buffer;
toString(encoding?: string): string;
}
}

export {}
Loading