core(script-treemap-data): default config #12494
Conversation
|
love the rename btw |
lighthouse-treemap/app/src/main.js
Outdated
| options.lhr.audits['script-treemap-data'].details); | ||
| if (!treemapDebugData || !treemapDebugData.treemapData) { | ||
| if (!scriptTreemapData || !scriptTreemapData.nodes) { |
There was a problem hiding this comment.
worth accepting both? I don't suppose we expect many treemap reports to have made it out into the wild thusfar, but it would be nice if they weren't bricked (or maybe file issue to add treemap UI message about compat when an unrecognized payload comes in?) :)
There was a problem hiding this comment.
worth accepting both? I don't suppose we expect many treemap reports to have made it out into the wild thusfar
I think it isn't because I think there are none. I shared some but it was as a standalone HTML file.
me neither. stumped. |
|
it's the |
|
@brendankenny may have a workaround. otherwise, can use EDIT: it's more complex than this. |
|
I really can't reproduce this, even in a basically identical branch. The details types are fully discriminated by the required |
|
FWIW the error sounds similar to #12280, but that case was enabled by types with all-optional properties, which (again) shouldn't be possible with |
| options.lhr.audits['script-treemap-data'].details); | ||
| if (!treemapDebugData || !treemapDebugData.treemapData) { | ||
| if (!scriptTreemapData || scriptTreemapData.type !== 'treemap-data') { |
There was a problem hiding this comment.
good call, and this check means the LH.Audit.Details.TreemapData cast above can be dropped too
There was a problem hiding this comment.
That caused some weird error... Can you try it and see?
There was a problem hiding this comment.
That caused some weird error
ah, perfect, because finally I can see it :)
Looks like this was from my idea in #12483 (comment)
In theory LH.RawIcu<LH.FormattedIcu<Audit.Details>> should resolve to an identity transformation and leave Audit.Details alone. However when I suggested we add dom?: HTMLElement to LH.Audit.Details.TreemapData, that ends up unleashing the zalgo and really putting the test to LH.RawIcu<LH.FormattedIcu<>> actually being an identity transformation...and it breaks. If you add "noErrorTruncation": true to our tsconfig you end up with a very fun error message. I would post it here under <details> but it's 1.7MiB long, repeated a second time in the tsc output for good measure.
A selection from our Audit.Details monstrosity:
...
spellcheck: boolean;
title: string | IcuMessage;
translate: boolean;
click: IcuMessage | {};
readonly classList: { [x: number]: string | IcuMessage; readonly length: number; value: string | IcuMessage; toString: IcuMessage | {}; add: IcuMessage | {}; contains: IcuMessage | {}; item: IcuMessage | {}; remove: IcuMessage | {}; replace: IcuMessage | {}; supports: IcuMessage | {}; toggle: IcuMessage | {}; forEach: IcuMessage | {}; entries: IcuMessage | {}; keys: IcuMessage | {}; values: IcuMessage | {}; };
className: string | IcuMessage;
readonly clientHeight: number
strokeMiterlimit: string | IcuMessage;
onpointerup: IcuMessage | {} | null;
onsecuritypolicyviolation: IcuMessage | {} | null;
...(in fairness, RawIcu/FormattedIcu were only ever meant to run on JSON-ish data, not anything with methods, among other things :)
I think the best solution is just to keep dom out of the audit details type (since it can never be in the LHR anyways) and have TreemapUtil.walk() return LH.Treemap.Node & {dom?: HTMLElement} so that's still typed.
There was a problem hiding this comment.
wonder why we both had trouble seeing this locally initially... maybe the type calculation was too massive
lighthouse-treemap/app/src/util.js
Outdated
| * @param {string[]=} path | ||
| */ | ||
| static walk(node, fn, path) { | ||
| if (!path) path = []; | ||
| path.push(node.name); | ||
|
|
||
| fn(node, path); | ||
| fn(/** @type {NodeWithElement} */ (node), path); |
There was a problem hiding this comment.
isn't this not necessary if L16 is @param {NodeWithElement} node ?
There was a problem hiding this comment.
oh right, because optional.
There was a problem hiding this comment.
oh, I was also assuming that was coming from the webtreemap library so is already augmented?
ref #11254
cc @brendankenny I also made a new Details type.