misc(treemap): elide origin from url if same as requestedUrl #12598
Conversation
| if (!dataRow.bundleNode) return ''; | ||
|
|
||
| return `${dataRow.bundleNode.name} (bundle) ${dataRow.name}`; | ||
| return `${dataRow.bundleNode.name} ${dataRow.name}`; |
There was a problem hiding this comment.
this (bundle) tag was doubling up–already present in table datarow name.
lighthouse-treemap/app/src/main.js
Outdated
| for (const node of this.depthOneNodesByGroup.scripts) { | ||
| const url = new URL(node.name); | ||
| node.name = TreemapUtil.elideUrl(url, this.documentUrl); | ||
| if (url.href === this.documentUrl.href) { |
There was a problem hiding this comment.
This works as temporary solution, but misses the case of iframes's inline JS. Haven't ever tested something like that so no idea if the app even works :)
lighthouse-treemap/app/src/main.js
Outdated
| // to elide common parts of the URL so text fits better in the UI. | ||
| for (const node of this.depthOneNodesByGroup.scripts) { | ||
| const url = new URL(node.name); | ||
| node.name = TreemapUtil.elideUrl(url, this.documentUrl); |
There was a problem hiding this comment.
I feel pretty good that these strings should be valid URLs, but should we do try/catches just in case?
There was a problem hiding this comment.
failure mode is that the whole treemap doesn't render, right?
let's do a try/catch :)
patrickhulce
left a comment
patrickhulce
left a comment
There was a problem hiding this comment.
I did this in the Treemap UI instead of script-treemap-data because I thought it'd be nice to preserve the property that the names in the LHR are valid URLs.
Good call, agreed 👍
I gotta say, as a third-party observer to a site, I'm not sure I was able to get much more out of each URL than before, but I suppose the site owner might be more familiar with all the paths.
lighthouse-treemap/app/src/main.js
Outdated
| // to elide common parts of the URL so text fits better in the UI. | ||
| for (const node of this.depthOneNodesByGroup.scripts) { | ||
| const url = new URL(node.name); | ||
| node.name = TreemapUtil.elideUrl(url, this.documentUrl); |
There was a problem hiding this comment.
failure mode is that the whole treemap doesn't render, right?
let's do a try/catch :)
lighthouse-treemap/app/src/util.js
Outdated
| * @param {URL} url | ||
| * @param {URL} fromRelativeUrl | ||
| */ | ||
| static elideUrl(url, fromRelativeUrl) { |
There was a problem hiding this comment.
elideSameOrigin sounds nice
|
|
||
| /** | ||
| * @param {URL} url | ||
| * @param {URL} fromRelativeUrl |
There was a problem hiding this comment.
should this just pass in the origin directly instead? I feel like it's a little clearer what's happening and it's not the "elide the whole URL" behavior from before.
There was a problem hiding this comment.
passing URLs around is a nice pre-condition that the origin/url components are actually in the right format. altho if these were both strings the try/catch would be more contained..
There was a problem hiding this comment.
I agree for the url param should be a URL 👍 (unless you want for try/catch reasons)
My rationale for this second base parameter is that we're only using the origin component, and I think it'd make it clearer which was the base/match argument and which was being elided from the caller. Not a big deal though, can be solved via renaming elideSameOrigin SGTM
| } | ||
|
|
||
| // Elide the document URL. | ||
| if (name.startsWith(this.currentTreemapRoot.name)) { |
There was a problem hiding this comment.
so this wasn't working?
There was a problem hiding this comment.
no, it was, it just isn't needed now because the eliding happens elsewhere. this was just for the table rows.
3 participants
The depth one script nodes have really long names because they are the entire URL of the network resource. We should elide if they share the same origin as the requested URL.
I did this in the Treemap UI instead of
script-treemap-databecause I thought it'd be nice to preserve the property that the names in the LHR are valid URLs.Also added an
(inline)tag to make the inline JS node apparent.https://lighthouse-git-tm-url-elide-googlechrome.vercel.app/gh-pages/treemap/?debug
https://lighthouse-git-tm-url-elide-googlechrome.vercel.app/gh-pages/treemap/?gist=682f30fc539d16905557e8ce092adecf
ref #11254