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: unknown details type #9557

Merged
merged 7 commits into from
Sep 14, 2019
Merged
Show file tree
Hide file tree
Changes from 3 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
25 changes: 20 additions & 5 deletions lighthouse-core/report/html/renderer/details-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,10 @@ class DetailsRenderer {
return null;

default: {
// @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
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
// with new unexpected detail types.
// @ts-ignore
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
return this._renderUnknown(details.type, details);
}
}
}
Expand Down Expand Up @@ -188,6 +189,20 @@ class DetailsRenderer {
return element;
}

/**
* @param {string} type
* @param {*} value
*/
_renderUnknown(type, value) {
// eslint-disable-next-line no-console
console.error(`Unknown valueType: ${type}`, value);
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
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.`;
Copy link
Member

Choose a reason for hiding this comment

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

`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.`

Copy link
Member

Choose a reason for hiding this comment

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

Can we not include the Ruh roh! please :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Inside a table may be a lost cause regardless

yea, I guess this is fine then.

Copy link
Member

Choose a reason for hiding this comment

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

this should be on Util.UIStrings, probably?

Copy link
Member

Choose a reason for hiding this comment

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

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.

this._dom.createChildOf(element, 'pre').textContent = JSON.stringify(value, null, 2);
return element;
}

/**
* Render a details item value for embedding in a table. Renders the value
* based on the heading's valueType, unless the value itself has a `type`
Expand Down Expand Up @@ -218,7 +233,7 @@ class DetailsRenderer {
return this.renderTextURL(value.value);
}
default: {
throw new Error(`Unknown valueType: ${value.type}`);
return this._renderUnknown(value.type, value);
}
}
}
Expand Down Expand Up @@ -267,7 +282,7 @@ class DetailsRenderer {
}
}
default: {
throw new Error(`Unknown valueType: ${heading.valueType}`);
return this._renderUnknown(heading.valueType, value);
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions lighthouse-core/report/html/report-styles.css
Original file line number Diff line number Diff line change
Expand Up @@ -1391,6 +1391,11 @@
display: block;
}

.lh-unknown pre {
overflow: scroll;
border: solid 1px var(--color-gray-200);
}

.lh-text__url > a {
color: inherit;
text-decoration: none;
Expand Down
26 changes: 20 additions & 6 deletions lighthouse-core/test/report/html/renderer/details-renderer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -194,14 +194,18 @@ describe('DetailsRenderer', () => {
assert.strictEqual(diagnosticEl, null);
});

it('throws on unknown details type', () => {
it('renders an unknown details type', () => {
// Disallowed by type system, but test that we get an error message out just in case.
const details = {
type: 'imaginary',
items: 5,
};

assert.throws(() => renderer.render(details), /^Error: Unknown type: imaginary$/);
const el = renderer.render(details);
const summaryEl = el.querySelector('summary').textContent;
assert.strictEqual(summaryEl,
'Details type \'imaginary\' unrecognized by this version of the report renderer.');
assert.strictEqual(el.lastChild.textContent, JSON.stringify(details, null, 2));
});
});

Expand Down Expand Up @@ -451,18 +455,23 @@ describe('DetailsRenderer', () => {
assert.strictEqual(codeItemEl.innerHTML, '<pre class="lh-code">invalid-url://example.com/</pre>');
});

it('throws on unknown heading itemType', () => {
it('renders an unknown heading itemType', () => {
// Disallowed by type system, but test that we get an error message out just in case.
const details = {
type: 'table',
headings: [{key: 'content', itemType: 'notRealValueType', text: 'Heading'}],
items: [{content: 'some string'}],
};

assert.throws(() => renderer.render(details), /^Error: Unknown valueType: notRealValueType$/);
const el = renderer.render(details);
const unknownEl = el.querySelector('td.lh-table-column--notRealValueType .lh-unknown');
const summaryEl = unknownEl.querySelector('summary').textContent;
assert.strictEqual(summaryEl,
'Details type \'notRealValueType\' unrecognized by this version of the report renderer.');
assert.strictEqual(unknownEl.lastChild.textContent, '"some string"');
});

it('throws on unknown item object type', () => {
it('renders an unknown item object type', () => {
// Disallowed by type system, but test that we get an error message out just in case.
const item = {
type: 'imaginaryItem',
Expand All @@ -475,7 +484,12 @@ describe('DetailsRenderer', () => {
items: [{content: item}],
};

assert.throws(() => renderer.render(details), /^Error: Unknown valueType: imaginaryItem$/);
const el = renderer.render(details);
const unknownEl = el.querySelector('td.lh-table-column--url .lh-unknown');
const summaryEl = unknownEl.querySelector('summary').textContent;
assert.strictEqual(summaryEl,
'Details type \'imaginaryItem\' unrecognized by this version of the report renderer.');
assert.strictEqual(unknownEl.lastChild.textContent, JSON.stringify(item, null, 2));
});

it('uses the item\'s type over the heading type', () => {
Expand Down