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

core(script-treemap-data): default config #12494

Merged
merged 11 commits into from
May 18, 2021
Merged

core(script-treemap-data): default config #12494

merged 11 commits into from
May 18, 2021

Conversation

connorjclark
Copy link
Collaborator

ref #11254

cc @brendankenny I also made a new Details type.

@connorjclark connorjclark requested a review from a team as a code owner May 17, 2021 21:02
@connorjclark connorjclark requested review from patrickhulce and removed request for a team May 17, 2021 21:02
@google-cla google-cla bot added the cla: yes label May 17, 2021
@paulirish
Copy link
Member

love the rename btw

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

🌴 🗺️, 🚢 💨 🍾 🎉

LGTM

I couldn't reproduce that typescript error locally :/ and bummer the e2e flow couldn't be tested with the required viewer-side change (that scared me for second 😆 ), but seems right to me!

options.lhr.audits['script-treemap-data'].details);
if (!treemapDebugData || !treemapDebugData.treemapData) {
if (!scriptTreemapData || !scriptTreemapData.nodes) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?) :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

@connorjclark
Copy link
Collaborator Author

I couldn't reproduce that typescript error locally

me neither. stumped.

@connorjclark
Copy link
Collaborator Author

it's the nodes, a property in both screenshot and treemap data ...

@connorjclark
Copy link
Collaborator Author

connorjclark commented May 18, 2021

@brendankenny may have a workaround. otherwise, can use treemapNodes. sad.

EDIT: it's more complex than this.

@brendankenny
Copy link
Member

I really can't reproduce this, even in a basically identical branch. The details types are fully discriminated by the required type, there's no reason they should need different property names otherwise. It would be nice to be able to repro this at all to see what's happening...

@brendankenny
Copy link
Member

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 LH.Audit.Details.

options.lhr.audits['script-treemap-data'].details);
if (!treemapDebugData || !treemapDebugData.treemapData) {
if (!scriptTreemapData || scriptTreemapData.type !== 'treemap-data') {
Copy link
Member

Choose a reason for hiding this comment

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

good call, and this check means the LH.Audit.Details.TreemapData cast above can be dropped too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That caused some weird error... Can you try it and see?

Copy link
Member

@brendankenny brendankenny May 18, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wonder why we both had trouble seeing this locally initially... maybe the type calculation was too massive

* @param {string[]=} path
*/
static walk(node, fn, path) {
if (!path) path = [];
path.push(node.name);

fn(node, path);
fn(/** @type {NodeWithElement} */ (node), path);
Copy link
Member

Choose a reason for hiding this comment

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

isn't this not necessary if L16 is @param {NodeWithElement} node ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh right, because optional.

Copy link
Member

Choose a reason for hiding this comment

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

oh, I was also assuming that was coming from the webtreemap library so is already augmented?

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

Nice! Like the new audit details type.

LGTMtoo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants