-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
Changes from 2 commits
79ea450
0503bfa
3abbbe8
73841ca
fe925e9
e6bb497
fa433be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,9 +63,9 @@ 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 with new unexpected detail types. | ||
// @ts-ignore | ||
return this._renderUnknown(details.type, details); | ||
} | ||
} | ||
} | ||
|
@@ -188,6 +188,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.`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we not include the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yea, I guess this is fine then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
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` | ||
|
@@ -218,7 +232,7 @@ class DetailsRenderer { | |
return this.renderTextURL(value.value); | ||
} | ||
default: { | ||
throw new Error(`Unknown valueType: ${value.type}`); | ||
return this._renderUnknown(value.type, value); | ||
} | ||
} | ||
} | ||
|
@@ -267,7 +281,7 @@ class DetailsRenderer { | |
} | ||
} | ||
default: { | ||
throw new Error(`Unknown valueType: ${heading.valueType}`); | ||
return this._renderUnknown(heading.valueType, value); | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@exterkamp hates indentation, apparently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya he do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rt, don't like it