-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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: add onViewTrace to renderer options #13528
Conversation
report/renderer/api.js
Outdated
@@ -12,6 +12,9 @@ import {ReportUIFeatures} from '../renderer/report-ui-features.js'; | |||
import {CategoryRenderer} from './category-renderer.js'; | |||
import {DetailsRenderer} from './details-renderer.js'; | |||
|
|||
/** @type {WeakMap<HTMLElement, ReportUIFeatures>} */ | |||
const rootElToReportUIFeatures = new WeakMap(); |
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.
Could you add some tests for this?
Storing the renderer state in globals can lead to lingering instances between test cases. Probably won't be a problem here since you use a map, but should still keep that in mind.
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.
What sort of test? Every call to render will make a new element, and so a new key->value pair in this map.
ahhh this tile has zero tests eh? kinda makes since, they api surface and methods are pretty minimal, and each individual component is pretty well tested. Not quite certain what tests should be added that wouldn't just be duplicates of existing tests.
one test I thought of is just doing renderReport
and then addButton
then confirming the button is in the el
. And doing a second report render and confirming the same.
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.
I was thinking of tests that verify the association between a rootEl
and an instance of ReportUIFeatures
. The one you mentioned would work.
report/renderer/api.js
Outdated
* @param {Parameters<ReportUIFeatures['addButton']>[0]} opts | ||
*/ | ||
function addButton(rootEl, opts) { | ||
const features = rootElToReportUIFeatures.get(rootEl); |
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.
We could get away with creating a new instance of ReportUIFeatures
in this function since ReportUIFeatures.addButton
only depends on the _dom
member of ReportUIFeatures
.
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.
This does work, however you have to dig through multiple class constructors to confirm there aren't any side effects from creates a new ReportUIFeatures, which suggests that this could be a foot-gun if those constructors began doing something other than rebinding a bunch of methods ... makes me lean towards keeping the map around. Semantically it makes since that there should only ever be one instance. wdyt?
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.
Worth discussing the API since this wasn't part of #12772? Something like
/** If defined, adds a View Trace button to the performance category. When clicked, will run this function. */
onViewTrace?: () => void;
fits well with the rest of the API and avoids any global state, keeping track of the rootEl, etc.
I agree, this fits better with the rest of the API. Leaves UX implementation details up to the report renderer. SGTM. |
hmmm, it would make sense to do |
what about adding this to Renderer.Options:
|
It already is a closure though, so that wouldn't be a change? But it would just need to close over the trace, not be a wrapper around
If the pro/con is a single function that matches the rest of the API vs support for placing arbitrary buttons in the perf category, I'd definitely vote for putting off the buttons API until we need a second one. |
Since both the if (this._opts._internalViewTraceButton) {
this.addButton(this._opts._internalViewTraceButton);
} This is what I meant by saying it would just be a wrapper around This might as well be Anyhow, I think marking this option as |
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.
wow. well that's a smaller diff!
sgtm!
In general I agree with @brendankenny that we don't need a generalized API for adding buttons, and we already have specialized options for adding specific buttons in the flow report. lighthouse/flow-report/types/flow-report.d.ts Lines 24 to 25 in 233b997
There is also
At least for consistency I would prefer an
I think it's better to move this translated string and any UX details behind the report renderer anyway. That we we aren't copying the
What more does it need? |
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.
Very cool, thanks Connor!
DevTools needs this to add the View Trace button https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/panels/lighthouse/LighthouseReportRenderer.ts;l=56;drc=93a4454b7c558d6ca748c718167bc4aa592eaf63