report: handle missing credits when rendering meta block #13350
Conversation
report/renderer/report-renderer.js
Outdated
| @@ -127,7 +127,7 @@ export class ReportRenderer { | |||
| : 'Chromium'; | |||
| const channel = report.configSettings.channel; | |||
| const benchmarkIndex = report.environment.benchmarkIndex.toFixed(0); | |||
| const axeVersion = report.environment.credits['axe-core']; | |||
| const axeVersion = report.environment.credits && report.environment.credits['axe-core']; | |||
There was a problem hiding this comment.
| const axeVersion = report.environment.credits && report.environment.credits['axe-core']; | |
| const axeVersion = report.environment.credits?.['axe-core']; |
There was a problem hiding this comment.
Instead, shouldn't we fix this in prepareReportResult? I'd expect the renderer to break with older LHRs w/o credits.
There was a problem hiding this comment.
i mean... i'd love that, but don't we have an incompatibility in our tooling stack?
(perhaps it was browserify?)
even if we're 100% gtg here, i'd much rather do this ?. upgrade in a bulk-change PR than introduce it with this one PR that i need for v9.
There was a problem hiding this comment.
before #13125 the renderer also did a check for report.environment.credits before accessing, so I'd personally be fine with keeping it local. prepareReportResult seems like it should be reserved for things we really need as opposed to more optional metadata.
We should probably make the property optional to make sure it's always checked, though.
There was a problem hiding this comment.
i mean... i'd love that, but don't we have an incompatibility in our tooling stack?
(perhaps it was browserify?)
it was browserify, but it would also still be fine because this is in the renderer, so it's browser only (and node for a unit test without any bundling)
even if we're 100% gtg here, i'd much rather do this ?. upgrade in a bulk-change PR than introduce it with this one PR that i need for v9.
haha, a mass change sounds worse to me rather than starting to use it as needed :)
There was a problem hiding this comment.
We should probably make the property optional to make sure it's always checked, though.
this is basically Report: add more robust handling of default property values stripped by LHR proto · #12868
it was browserify, but it would also still be fine because this is in the renderer, so it's browser only (and node for a unit test without any bundling)
k thanks for the confirmation.
There was a problem hiding this comment.
We should probably make the property optional to make sure it's always checked, though.
this is basically Report: add more robust handling of default property values stripped by LHR proto · #12868
Yeah, fair point. I guess I was thinking differently about it because it was meant to be optionally handled from the start, so we should have just typed it like that instead of setting it as required but handling the optional fallback in the renderer. Without the type the next refactor was bound to remove the check.
We should probably write down guidance for ourselves for both robust handling of trips through proto space and easing compatibility with old LHRs so we don't forget it all every 18 months or so :)
There was a problem hiding this comment.
The separate consideration of some types to be optional on the LHR (when they really would always exist in the latest LH run) is confusing IMO. The "fixup" is simple enough (empty object for credits, done in the same place with fixup all other things about older LHRs before rendering), although for this property in particular that still leaves us needing to check the existence of the axe-core key (can we change the type to Record<string, string|undefined>?).
Co-authored-by: Connor Clark <[email protected]>
connorjclark
changed the title
LHR PSI json doesn't have this field currently.
I'm adding it but we need to make sure this doesnt throw in the meantime.
this'll break new /measure (using v9 renderer) in the meantime.