report: unknown details type #9557

connorjclark · 2019-08-14T19:11:20Z

We currently throw on unknown details types. This means that any new details type is a breaking change.

So instead, let's just show the data with a warning message.

connorjclark · 2019-08-14T19:19:58Z

Maybe summary should be shorter (just "Unknown"). Otherwise would make small, unknown columns really really big.

paulirish · 2019-09-13T20:08:13Z

oof yah. this hurts.

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

exterkamp

LGTM.

This seems like a positive change for renderers in the future, but should be a no-op now afaict.

Co-Authored-By: Shane Exterkamp <shaneexterkamp5@gmail.com>

paulirish

yah good call on this.

have some revised text for the error msg.

paulirish · 2019-09-13T20:12:32Z

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

+    console.error(`Unknown valueType: ${type}`, value);
+    const element = this._dom.createElement('details', 'lh-unknown');
+    this._dom.createChildOf(element, 'summary').textContent =
+      `Details type '${type}' unrecognized by this version of the report renderer.`;


`Ruh roh! 😿 We don't know how to render audit details of type \`${type}\`. The Lighthouse version that collected this data is likely newer than the Lighthouse version of the report renderer. Expand for the raw JSON.`

Can we not include the Ruh roh! please :)

Having long text here will be bad if the unknown type is in a table. Thoughts on just "Unknown" -> expand -> see the message you have here followed by the JSON?

Having long text here will be bad if the unknown type is in a table. Thoughts on just "Unknown" -> expand -> see the message you have here followed by the JSON?

how does the pre-box look in that case (especially if there's one on each row or whatever)? Inside a table may be a lost cause regardless

Inside a table may be a lost cause regardless

yea, I guess this is fine then.

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

brendankenny · 2019-09-13T20:32:21Z

oof yah. this hurts.

I mean, there was a reason for this. After this no one is going to notice if you accidentally trick tsc with a cast and try to have a criticalrequestchains or filmstripz details type because the result is collapsed in an audit somewhere.

Same with web.dev. Why update the report renderer if it's working...meanwhile after source-location anyone using deprecations and font-size audits are going to get unstyled json.

The actual right call is to only add details types on breaking changes, but if we're not willing to do that (yet), then this is probably the best we can do.

brendankenny · 2019-09-13T20:33:20Z

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

-        // @ts-ignore tsc thinks this unreachable, but ts-ignore for error message just in case.
-        const detailsType = details.type;
-        throw new Error(`Unknown type: ${detailsType}`);
+// Tsc thinks this is unreachable. But renderers should be forward compatible with new unexpected detail types.


@exterkamp hates indentation, apparently

rt, don't like it

brendankenny

reluctant LGTM but this will be an improvement :)

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

brendankenny · 2019-09-13T20:44:05Z

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

+    console.error(`Unknown valueType: ${type}`, value);
+    const element = this._dom.createElement('details', 'lh-unknown');
+    this._dom.createChildOf(element, 'summary').textContent =
+      `Details type '${type}' unrecognized by this version of the report renderer.`;


this should be on Util.UIStrings, probably?

It can be if we modify it to not use ICU. If we want to add a {type} we need someway of jamming i18n into the report. #9166

I suggest leaving this not translated, add a TODO referring to #7238, and then we can handle it in a follow up that allows ICU in the report.

Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

Co-Authored-By: Paul Irish <paulirish@google.com>

patrickhulce

❤️ it

connorjclark · 2019-09-14T00:19:31Z

After this no one is going to notice if you accidentally trick tsc with a cast and try to have a criticalrequestchains or filmstripz details type because the result is collapsed in an audit somewhere.

do we have any tests that check for console error messages in the report?

report: unknown details type

79ea450

connorjclark requested a review from paulirish as a code owner August 14, 2019 19:11

googlebot added the cla: yes label Aug 14, 2019

exterkamp reviewed Sep 13, 2019

View reviewed changes

lighthouse-core/report/html/renderer/details-renderer.js Outdated Show resolved Hide resolved

exterkamp approved these changes Sep 13, 2019

View reviewed changes

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

0503bfa

Co-Authored-By: Shane Exterkamp <shaneexterkamp5@gmail.com>

vercel bot deployed to staging September 13, 2019 20:18 View deployment

paulirish approved these changes Sep 13, 2019

View reviewed changes

brendankenny reviewed Sep 13, 2019

View reviewed changes

fix

3abbbe8

vercel bot deployed to staging September 13, 2019 20:35 View deployment

brendankenny approved these changes Sep 13, 2019

View reviewed changes

Apply suggestions from code review

73841ca

Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

vercel bot temporarily deployed to staging September 13, 2019 20:57 Inactive

Apply suggestions from code review

fe925e9

Co-Authored-By: Paul Irish <paulirish@google.com>

vercel bot deployed to staging September 13, 2019 20:57 View deployment

errormsg

e6bb497

vercel bot deployed to staging September 13, 2019 21:10 View deployment

patrickhulce approved these changes Sep 13, 2019

View reviewed changes

fix tests

fa433be

vercel bot deployed to staging September 13, 2019 21:15 View deployment

connorjclark merged commit 1a45e86 into master Sep 14, 2019

connorjclark deleted the unknown-details branch September 14, 2019 00:20

paulirish pushed a commit that referenced this pull request Nov 6, 2019

report: unknown details type (#9557)

6f5ff16

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: unknown details type #9557

report: unknown details type #9557

connorjclark commented Aug 14, 2019

connorjclark commented Aug 14, 2019 •

edited

Loading

paulirish commented Sep 13, 2019

exterkamp left a comment

paulirish left a comment

paulirish Sep 13, 2019

brendankenny Sep 13, 2019

connorjclark Sep 13, 2019

brendankenny Sep 13, 2019

connorjclark Sep 13, 2019

brendankenny commented Sep 13, 2019 •

edited

Loading

brendankenny Sep 13, 2019

connorjclark Sep 13, 2019

exterkamp Sep 13, 2019

brendankenny left a comment

brendankenny Sep 13, 2019

exterkamp Sep 13, 2019

patrickhulce left a comment

connorjclark commented Sep 14, 2019

report: unknown details type #9557

report: unknown details type #9557

Conversation

connorjclark commented Aug 14, 2019

connorjclark commented Aug 14, 2019 • edited Loading

paulirish commented Sep 13, 2019

exterkamp left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Sep 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

connorjclark commented Sep 14, 2019

connorjclark commented Aug 14, 2019 •

edited

Loading

brendankenny commented Sep 13, 2019 •

edited

Loading