core(tsc): add tsc type checking to report #5195

brendankenny · 2018-05-11T22:57:47Z

In the process of updating to the new details format, it became obvious that our details type hierarchy is wrong in a few ways in the report renderer, and it was difficult to track how just using Closure. This drops Closure and moves to use tsc for type checking.

A minimum effort was made to update the code for the new checker (e.g. it still uses the old typedefs instead of using LHR types yet) since changes are still in progress and it'll require some more extensive surgery to get everything correct.

In order to use the typedefs across files I updated our version of tsc so we can use the new
/** @typedef {import('./report-renderer.js').GroupJSON} GroupJSON */
format so we can
a) not require() a file just for its types and (risk circular require issues) and
b) use typedefs across files

sorry for the PR size. On the plus side, it's almost all jsdoc changes :)

brendankenny · 2018-05-11T22:58:06Z

lighthouse-core/lib/dependency-graph/simulator/simulator.js

@@ -317,7 +317,7 @@ class Simulator {
  simulate(graph, options) {
    options = Object.assign({flexibleOrdering: false}, options);
    // initialize the necessary data containers
-    this._flexibleOrdering = options.flexibleOrdering;
+    this._flexibleOrdering = !!options.flexibleOrdering;


updated tsc was stricter here

brendankenny · 2018-05-11T22:59:50Z

tsconfig.json

@@ -15,15 +15,18 @@
    "diagnostics": true
  },
  "include": [
+    // TODO(bckenny): unnecessary workaround until https://github.com/Microsoft/TypeScript/issues/24062
+    "node_modules/@types/node/index.d.ts",


annoying, hopefully temporary issue where the compiler thinks that EventEmitter has a duplicate identifier. Manually putting the node types first fixes this for now.

brendankenny · 2018-05-11T23:00:48Z

typings/artifacts.d.ts

@@ -4,8 +4,8 @@
 * 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.
 */

-import * as parseManifest from '../lighthouse-core/lib/manifest-parser.js';
-import * as _LanternSimulator from '../lighthouse-core/lib/dependency-graph/simulator/simulator.js';
+import parseManifest = require('../lighthouse-core/lib/manifest-parser.js');


turns out we shouldn't be trying to import * as a commonjs module like this (and it's broken in newer versions of the compiler), so switched to the proprietary import x = require('x.js') (works only in typescript code)

is this just for commonjs cases where you're not using a declare module? got a link to somewhere that has the offish word :)

the offish word :)

Probably here in the handbook, but there's a better explanation of the reasoning here from one of the typescript engineers:

These are mostly equivalent, but import * has some restrictions that import ... = require doesn't. import * as creates an identifier that is a module object, emphasis on object. According to the ES6 spec, this object is never callable or newable - it only has properties.`

https://stackoverflow.com/a/35706271

In our case, it appears they also extend that to importing the type of the module's export, as our old way is broken with the more recent releases of typescript. In fact, he ends it with

In some runtime+transpilation environments this might happen to work anyway, but it might break at any point in the future without warning, which will make you sad.

:)

brendankenny · 2018-05-11T23:02:47Z

typings/html-renderer.d.ts

+  var PerformanceCategoryRenderer: typeof _PerformanceCategoryRenderer;
+  var ReportRenderer: typeof _ReportRenderer;
+  var ReportUIFeatures: typeof _ReportUIFeatures;
+  var Util: typeof _Util;


these add these to the global context for all of our code, even though we only depend on them being global inside the renderer.

An alternative would be to make a second tsconfig.json file just for the renderer and compile the main project and the renderer separately. Not sure if it matters right now (but it might be more of a pain to have two separate tsc runs, don't know)

patrickhulce

WFM! 🔪 ❤️ ☠️ 🚧

patrickhulce · 2018-05-11T23:48:00Z

lighthouse-core/report/html/renderer/report-renderer.js

+      reportCategories: Array<CategoryJSON>,
+      categoryGroups: Object<string, GroupJSON>,
+      configSettings: LH.Config.Settings,
+  }} ReportJSON


it'd be nice to switch these to the @property syntax later on so we get some actual syntax highlighting :)

t'd be nice to switch these to the @Property syntax later on

agreed, though I'm hoping the majority of the use of these will become references to the LHR types :) (it was purely laziness that I didn't convert these...I can if it would be better)

patrickhulce · 2018-05-11T23:49:00Z

typings/artifacts.d.ts

@@ -4,8 +4,8 @@
 * 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.
 */

-import * as parseManifest from '../lighthouse-core/lib/manifest-parser.js';
-import * as _LanternSimulator from '../lighthouse-core/lib/dependency-graph/simulator/simulator.js';
+import parseManifest = require('../lighthouse-core/lib/manifest-parser.js');


is this just for commonjs cases where you're not using a declare module? got a link to somewhere that has the offish word :)

paulirish · 2018-05-12T01:14:07Z

This is super cool. :)

What's your proposal for devtools integration? Currently we include the renderer as part of all front_end typechecking.

brendankenny · 2018-05-14T19:03:12Z

What's your proposal for devtools integration? Currently we include the renderer as part of all front_end typechecking.

I'd need to take a look at the type checking step. Either it could be externs for the limited API (LHR, the major renderer classes) or we could do a full tsickle build step + whatever else cause tsickle is pretty limited

brendankenny · 2018-05-15T22:47:54Z

@paulirish yah? nah?

paulirish · 2018-05-16T00:30:58Z

let's get yarn compile-devtools working with this. then i'm good.

brendankenny · 2018-05-16T00:44:30Z

let's get yarn compile-devtools working with this.

well I tried and couldn't get it working with master, let alone the new branch, so maybe not worth it now

paulirish

lgtm

wooohoo we are free

brendankenny · 2018-05-17T17:29:07Z

I'll update to the tsc 2.9rc that was just released

brendankenny added 2 commits May 11, 2018 15:48

core(tsc): add tsc type checking to report

5d623c0

remove closure

9cce193

brendankenny requested review from paulirish and vinamratasingal-zz as code owners May 11, 2018 22:57

paulirish deployed to PR staging May 11, 2018 22:57 View deployment

brendankenny commented May 11, 2018

View reviewed changes

paulirish deployed to PR staging May 11, 2018 23:30 View deployment

patrickhulce approved these changes May 11, 2018

View reviewed changes

Merge branch 'master' into tsrender

068ef7b

paulirish deployed to PR staging May 14, 2018 19:06 View deployment

add null check

32d8590

paulirish deployed to PR staging May 14, 2018 19:30 View deployment

Merge branch 'master' into tsrender

6df2477

paulirish deployed to PR staging May 17, 2018 17:24 View deployment

paulirish approved these changes May 17, 2018

View reviewed changes

update tsc, last type fixes

0ac4627

paulirish deployed to PR staging May 17, 2018 18:54 View deployment

golden LHR

f634dd9

GoogleChrome deleted a comment from googlebot May 17, 2018

paulirish deployed to PR staging May 17, 2018 19:02 View deployment

brendankenny merged commit 4a046e0 into master May 17, 2018

brendankenny deleted the tsrender branch May 17, 2018 19:09

brendankenny mentioned this pull request May 18, 2019

Improve node selectors shown in the details table #4015

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(tsc): add tsc type checking to report #5195

core(tsc): add tsc type checking to report #5195

brendankenny commented May 11, 2018 •

edited

Loading

brendankenny May 11, 2018

brendankenny May 11, 2018 •

edited

Loading

brendankenny May 11, 2018 •

edited

Loading

patrickhulce May 11, 2018

brendankenny May 12, 2018

brendankenny May 11, 2018

patrickhulce left a comment

patrickhulce May 11, 2018

brendankenny May 11, 2018

patrickhulce May 11, 2018

paulirish commented May 12, 2018

brendankenny commented May 14, 2018

brendankenny commented May 15, 2018

paulirish commented May 16, 2018 •

edited

Loading

brendankenny commented May 16, 2018

paulirish left a comment

brendankenny commented May 17, 2018

core(tsc): add tsc type checking to report #5195

core(tsc): add tsc type checking to report #5195

Conversation

brendankenny commented May 11, 2018 • edited Loading

brendankenny May 11, 2018

Choose a reason for hiding this comment

brendankenny May 11, 2018 • edited Loading

Choose a reason for hiding this comment

brendankenny May 11, 2018 • edited Loading

Choose a reason for hiding this comment

patrickhulce May 11, 2018

Choose a reason for hiding this comment

brendankenny May 12, 2018

Choose a reason for hiding this comment

brendankenny May 11, 2018

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

patrickhulce May 11, 2018

Choose a reason for hiding this comment

brendankenny May 11, 2018

Choose a reason for hiding this comment

patrickhulce May 11, 2018

Choose a reason for hiding this comment

paulirish commented May 12, 2018

brendankenny commented May 14, 2018

brendankenny commented May 15, 2018

paulirish commented May 16, 2018 • edited Loading

brendankenny commented May 16, 2018

paulirish left a comment

Choose a reason for hiding this comment

brendankenny commented May 17, 2018

brendankenny commented May 11, 2018 •

edited

Loading

brendankenny May 11, 2018 •

edited

Loading

brendankenny May 11, 2018 •

edited

Loading

paulirish commented May 16, 2018 •

edited

Loading