add Closure type checking for report v2 #2043

brendankenny · 2017-04-20T06:18:58Z

@paulirish already took this most of the way there in #2002, this is just cleaning up the rest and hitting some pretty strict rules, which is nice. It has to be manually run right now (npm run closure)...I wasn't sure if I should enable it or not yet in CI.

Two things that might be controversial:

DetailsJSON has been flattened into basically a base class (DetailsJSON with a type and optional text field) and derived classes ListDetailsJSON, and CardDetailsJSON (names are terrible :). Inheritance isn't actually used, however.

Precipitating reason is that you can't do circular typedefs, so these weren't being checked at all. DevTools compilation didn't notice because it's not using the NewTypeInference engine (to be fair, we aren't either by default because of some lingering bugs). We could do an actual @interface, which handles circular references, but we aren't actually using the recursive nature of DetailsJSON anywhere and it really just makes it harder to figure out what type any particular place is dealing with. If every property on DetailsJSON was going to be optional except for type, and type determines which properties are defined on a particular instance and what function deals with rendering it...that sounds a whole lot like a separate type, so lets just call it that.

When dispatched from _render to their particular _render* method, they're cast to the specific type needed. Whole thing works because these are structural types, so as long as they fit the structure they're good to go.

For lists and cards I tried to narrow the property types to how they're actually used. _renderBlock is never actually used, so I have no information there, so I just deleted rather than deal with it. Next PR that needs blocks can add it back :)
As mentioned offline to some, I've replaced instances of context.querySelector(query) (which can return null if nothing is found) with DOM.find(context, query) which never returns null so we don't have to constantly assure closure that it's not null. We use it for finding stuff in templates, so if the target isn't found we either screwed up the template or the query, and DOM.find helpfully throws an exception in that case.

brendankenny · 2017-04-20T06:32:32Z

DevTools compilation didn't notice because it's not using the NewTypeInference engine (to be fair, we aren't either by default because of some lingering bugs).

new_type_inf is commented out here. NTI is way better at many things: inferring types, noticing where code is tightening a type, supporting more ES6 stuff, etc, but there are some things it doesn't support yet, and since it's stricter with "there's a mistake here" -> "that's an error" (as opposed to "well, you probably don't want me to be a bother"), we can't really use it yet.

I have used it to set up the typing in this PR, but disabled it for now. Uncomment that line to see its output. Some examples:

it doesn't support @implicitCast yet, which is needed to not see an error on setting textContent.
it gets confused by the default parameter on attrs = {} meaning the param can't be undefined anymore
it loses type information of what's in an array in the callback sent to Array.prototype.filter and when destructuring

Notably the old type inference also gets confused by the last two, it just doesn't complain, which is arguably worse.

The Closure Compiler team is doing a large push on NTI right now, most especially finishing the ES6->ES6 compiler so type checking can be done on the actual code instead of transpiled ES5 code, so I'm hoping that our particular bugs will be squashed in the next month or two.

Full NTI output:

> lighthouse@1.7.0-alpha.1 closure
> cd lighthouse-core && node closure/closure-type-checking.js

gulp-google-closure-compiler: /report/v2/renderer/details-renderer.js:52: WARNING - The right side in the assignment is not a subtype of the left side.
Expected : string
Found    : string|undefined
More details:
The found type is a union that includes an unexpected type: undefined
    element.textContent = text.text;
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

/report/v2/renderer/details-renderer.js:83: WARNING - Attempt to use nullable type Node|null.
      element.appendChild(this._dom.createElement('summary')).textContent = details.header.text;
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

/report/v2/renderer/details-renderer.js:94: WARNING - Attempt to use nullable type Node|null.
      card.appendChild(titleEl).textContent = item.title;
      ^^^^

/report/v2/renderer/details-renderer.js:95: WARNING - Attempt to use nullable type Node|null.
      card.appendChild(valueEl).textContent = item.value;
      ^^^^^^^^^^^^^^^^^^^^^^^^^

/report/v2/renderer/details-renderer.js:98: WARNING - Attempt to use nullable type Node|null.
        card.appendChild(targetEl).textContent = `target: ${item.target}`;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^

/report/v2/renderer/dom.js:40: WARNING - This GETPROP expression has the unknown type.
      element.className = className;
      ^^^^^^^^^^^^^^^^^

/report/v2/renderer/dom.js:43: WARNING - Attempt to use nullable type IObject<string,string|undefined>|undefined.
      const value = attrs[key];
                    ^^^^^

/report/v2/renderer/dom.js:77: WARNING - This GETPROP expression has the unknown type.
      const [preambleText, linkText, linkHref] = parts.splice(0, 3);
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

/report/v2/renderer/dom.js:77: WARNING - Violation: References to object props that are typed as `unknown` are discouraged. See https://github.com/google/closure-compiler/wiki/JS-Conformance-Framework#banunknowntypedclasspropsreferences
The property "value" on type "IIterableResult<?>"
      const [preambleText, linkText, linkHref] = parts.splice(0, 3);
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

/report/v2/renderer/dom.js:81: WARNING - This AND expression has the unknown type.
      if (linkText && linkHref) {
          ^^^^^^^^^^^^^^^^^^^^

/report/v2/renderer/report-renderer.js:182: WARNING - Property score never defined on any type in the program
    const passedAudits = category.audits.filter(audit => audit.score === 100);
                                                         ^^^^^^^^^^^

/report/v2/renderer/report-renderer.js:182: WARNING - This GETPROP expression has the unknown type.
    const passedAudits = category.audits.filter(audit => audit.score === 100);
                                                         ^^^^^^^^^^^

/report/v2/renderer/report-renderer.js:183: WARNING - This NAME expression has the unknown type.
    const nonPassedAudits = category.audits.filter(audit => !passedAudits.includes(audit));
                                                                                   ^^^^^

/report/v2/renderer/report-renderer.js:185: WARNING - This GETPROP expression has the unknown type.
    for (const audit of nonPassedAudits) {
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

/report/v2/renderer/report-renderer.js:185: WARNING - Violation: References to object props that are typed as `unknown` are discouraged. See https://github.com/google/closure-compiler/wiki/JS-Conformance-Framework#banunknowntypedclasspropsreferences
The property "value" on type "IIterableResult<?>"
    for (const audit of nonPassedAudits) {
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

/report/v2/renderer/report-renderer.js:197: WARNING - This GETPROP expression has the unknown type.
    for (const audit of passedAudits) {
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

/report/v2/renderer/report-renderer.js:197: WARNING - Violation: References to object props that are typed as `unknown` are discouraged. See https://github.com/google/closure-compiler/wiki/JS-Conformance-Framework#banunknowntypedclasspropsreferences
The property "value" on type "IIterableResult<?>"
    for (const audit of passedAudits) {
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

0 error(s), 17 warning(s), 96.4% typed

Closure compilation successful.

brendankenny · 2017-04-20T06:40:35Z

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

@@ -73,7 +75,7 @@ class ReportRenderer {
  renderReport(report) {
    try {
      return this._renderReport(report);
-    } catch (e) {
+    } catch (/** @type {!Error} */ e) {


this is an example of stuff not needed with NTI

patrickhulce

I'm not liking the idea of already nuking recursive rendering as that was kinda the point of this structure in the first place. Table will definitely need this and list should already need it for text.

patrickhulce · 2017-04-20T17:40:35Z

lighthouse-core/test/report/v2/renderer/details-renderer-test.js

@@ -95,31 +81,6 @@ describe('DetailsRenderer', () => {
      assert.equal(items.children.length, 3, 'did not render children');
    });

-    it('renders nested structures', () => {


why? even if we don't care about blocks we certainly care about text within lists

why? even if we don't care about blocks we certainly care about text within lists

oh, I didn't actually keep reading and see the list part :)

actually, without the block part it's just a duplicate of the renders lists with headers test above, so it's probably fine to remove and get more detailed list tests in #2019

brendankenny · 2017-04-20T19:44:07Z

I'm not liking the idea of already nuking recursive rendering as that was kinda the point of this structure in the first place. Table will definitely need this and list should already need it for text.

This PR was aiming to satisfy two constraints: do actual type checking and don't change how things are currently being used. And just to be clear, a DAG of typedefs works fine (e.g. lists have items that are text or urls or images or etc...just not items that are lists or things that contain lists).

Currently the only thing actually using the recursive structure was the unused _renderBlocks. No render method (including _renderBlocks) was treating the header as an actual DetailsJSON (nothing was even checking the type of header to make sure it was a text node, let alone pass it back to _render).

Only some types had items defined, and then you had to check at runtime if it was type cards or type list to know what was actually in those items. That's not (statically) checking types anymore if we call all those things by the same name. If every _render* method is going to be bespoke and only call back into _render for a limited number of other types, that's easily represented by a DAG.

And if and when we need them, we can switch over to actual record types/interfaces instead of typedefs to support circular references.

patrickhulce · 2017-04-20T21:41:08Z

No render method (including _renderBlocks) was treating the header as an actual DetailsJSON (nothing was even checking the type of header to make sure it was a text node, let alone pass it back to _render).

This is a bad thing, not something we should cement with type-checking that won't support the proper method.

If every _render* method is going to be bespoke and only call back into _render for a limited number of other types, that's easily represented by a DAG.

If this is really going to be the case then I misunderstood what direction details was headed and definitely do not like it 😆 I'll dismiss my review because I think I'm out of sync with how the new world of LH front-end is taking shape...

"I think I'm out of sync with how the new world of LH front-end is taking shape..."

brendankenny · 2017-04-20T21:44:48Z

If this is really going to be the case then I misunderstood what direction details was headed and definitely do not like it 😆 I'll dismiss my review because I think I'm out of sync with how the new world of LH front-end is taking shape...

yeah, there's probably a meta issue here: "wtf is details" :)

The current queue of report PRs just sends us further down this path, so we should probably figure that out first if we don't want to go that way

paulirish · 2017-04-20T22:41:16Z

lighthouse-core/report/v2/renderer/dom.js

@@ -56,7 +55,7 @@ class DOM {
   * @throws {Error}
   */
  cloneTemplate(selector, context) {
-    const template = context.querySelector(selector);
+    const template = /** @type {HTMLTemplateElement} */ (context.querySelector(selector));


compiling this PR in devtools i get this error:

.../devtools/front_end/audits2/lighthouse/renderer/dom.js:58 ERROR - Type "HTMLTemplateElement" nullability not marked explicitly with "?" (nullable) or "!" (non-nullable) const template = /** @type {HTMLTemplateElement} */ (context.querySelector(selector));

that's the only error though!

interesting! The devtools compiler must have an extra check for explicit nullability?

fixed! made it explicitly nullable so the following if statement makes sense :)

paulirish

nice pr. two proposals below.

yeah, there's probably a meta issue here: "wtf is details" :)
The current queue of report PRs just sends us further down this path, so we should probably figure that out first if we don't want to go that way

let's get this sorted out tomorrow.

paulirish · 2017-04-21T02:50:41Z

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

@@ -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');


need to use the instance because we can't rely on this global being present.
so i guess let's drop the static and use it as this._dom.find(...

also, suggestion on alternative method signature above...

paulirish · 2017-04-21T02:51:13Z

lighthouse-core/report/v2/renderer/dom.js

+   * @param {string} query
+   * @return {!Element}
+   */
+  static find(context, query) {


to be more like https://www.w3.org/TR/selectors-api2/ it would make more sense to do find(query, context).
i'd prefer it, as well.

paulirish · 2017-04-21T02:55:12Z

lighthouse-core/report/v2/renderer/dom.js

@@ -56,7 +55,7 @@ class DOM {
   * @throws {Error}
   */
  cloneTemplate(selector, context) {
-    const template = context.querySelector(selector);
+    const template = /** @type {HTMLTemplateElement} */ (context.querySelector(selector));


brendankenny · 2017-04-21T21:36:16Z

ok, find(query, context) now has good param order and is no longer static.

Also added closure compiler check into travis and npm test

brendankenny · 2017-04-21T21:45:00Z

Also added closure compiler check into travis

currently, at least, this is adding 5-6 seconds on travis, so currently tied with lint for our fastest CI test :)

paulirish · 2017-04-21T21:55:41Z

Yeah closure compilation is back! and on travis!

⛵️ ⛵️ ⛵️ ⛵️ ⛵️ ⛵️ ⛵️ ⛵️ ⛵️ ⛵️ ⛵️ ⛵️ ⛵️ ⛵️ ⛵️ ⛵️

brendankenny commented Apr 20, 2017

View reviewed changes

brendankenny force-pushed the report-closure branch 2 times, most recently from 4826cf8 to 4e07204 Compare April 20, 2017 06:44

patrickhulce previously requested changes Apr 20, 2017

View reviewed changes

paulirish reviewed Apr 20, 2017

View reviewed changes

brendankenny force-pushed the report-closure branch from 4e07204 to dedc2e2 Compare April 21, 2017 01:44

paulirish requested changes Apr 21, 2017

View reviewed changes

brendankenny added 3 commits April 21, 2017 14:30

initial closure type checking for report v2

725d1a3

explicit nullable query

415a1dc

DOM.find non static; enable report closure checking in tests

2285e5b

brendankenny force-pushed the report-closure branch from dedc2e2 to 2285e5b Compare April 21, 2017 21:33

paulirish approved these changes Apr 21, 2017

View reviewed changes

paulirish merged commit 2faa686 into master Apr 21, 2017

paulirish deleted the report-closure branch April 21, 2017 21:55

brendankenny mentioned this pull request Apr 17, 2024

misc: deduplicate all the dom helpers #15960

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Closure type checking for report v2 #2043

add Closure type checking for report v2 #2043

brendankenny commented Apr 20, 2017 •

edited

Loading

brendankenny commented Apr 20, 2017

brendankenny Apr 20, 2017

patrickhulce left a comment

patrickhulce Apr 20, 2017

brendankenny Apr 20, 2017

brendankenny Apr 20, 2017

brendankenny commented Apr 20, 2017

patrickhulce commented Apr 20, 2017

brendankenny commented Apr 20, 2017 •

edited

Loading

paulirish Apr 20, 2017 •

edited

Loading

brendankenny Apr 20, 2017

brendankenny Apr 21, 2017

paulirish Apr 21, 2017

paulirish left a comment

paulirish Apr 21, 2017

paulirish Apr 21, 2017

paulirish Apr 21, 2017

brendankenny commented Apr 21, 2017 •

edited by paulirish

Loading

brendankenny commented Apr 21, 2017

paulirish commented Apr 21, 2017

add Closure type checking for report v2 #2043

add Closure type checking for report v2 #2043

Conversation

brendankenny commented Apr 20, 2017 • edited Loading

brendankenny commented Apr 20, 2017

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Apr 20, 2017

patrickhulce commented Apr 20, 2017

brendankenny commented Apr 20, 2017 • edited Loading

paulirish Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Apr 21, 2017 • edited by paulirish Loading

brendankenny commented Apr 21, 2017

paulirish commented Apr 21, 2017

brendankenny commented Apr 20, 2017 •

edited

Loading

brendankenny commented Apr 20, 2017 •

edited

Loading

paulirish Apr 20, 2017 •

edited

Loading

brendankenny commented Apr 21, 2017 •

edited by paulirish

Loading