feat: enhanced display URLs #1793
Conversation
lighthouse-core/lib/url-shim.js
Outdated
| * @return {string} | ||
| */ | ||
| URL.getDisplayName = function getDisplayName(url) { | ||
| URL.getDisplayName = function getDisplayName(url, options) { |
There was a problem hiding this comment.
nit: options = {} default arg.
works as you have it though.
lighthouse-core/lib/url-shim.js
Outdated
| options = Object.assign({ | ||
| pathParts: 2, | ||
| removeQuery: true, | ||
| removeHost: true, |
There was a problem hiding this comment.
cant remember where we landed on trailing ,. Anyone?
There was a problem hiding this comment.
I've been slowly sneaking them in over time as I touch files :) I'm in favor of always enforcing for cleaner diffs
| @@ -0,0 +1 @@ | |||
| {"_":["https://www.theverge.com/"],"help":false,"version":false,"disable-storage-reset":false,"disableStorageReset":false,"disable-device-emulation":true,"disableDeviceEmulation":true,"disable-cpu-throttling":true,"disableCpuThrottling":true,"disable-network-throttling":true,"disableNetworkThrottling":true,"save-assets":false,"saveAssets":false,"save-artifacts":false,"saveArtifacts":false,"list-all-audits":false,"listAllAudits":false,"list-trace-categories":false,"listTraceCategories":false,"perf":false,"skip-autolaunch":false,"skipAutolaunch":false,"select-chrome":false,"selectChrome":false,"verbose":false,"quiet":false,"interactive":true,"output":"pretty","output-path":"stdout","outputPath":"stdout","port":9222,"max-wait-for-load":25000,"maxWaitForLoad":25000,"$0":"lighthouse-cli/index.js","logLevel":"info"} | |||
There was a problem hiding this comment.
uh I have no idea...something must have disappeared from .gitignore
lighthouse-core/lib/url-shim.js
Outdated
| pathParts: 2, | ||
| removeQuery: true, | ||
| removeHost: true, | ||
| }, options) |
lighthouse-core/lib/url-shim.js
Outdated
| // And grab the last two parts. | ||
| .split('/').slice(-2).join('/'); | ||
| name = parsed.pathname; | ||
| const parts = name.split('/') |
lighthouse-core/lib/url-shim.js
Outdated
| name = `${parsed.host}/${name.replace(/^\//, '')}`; | ||
| } | ||
| if (!options.removeQuery) { | ||
| name = `${name}${parsed.search}` |
lighthouse-core/lib/url-shim.js
Outdated
| if (name.length > MAX_LENGTH && name.includes('?')) { | ||
| const prevName = name; | ||
| // Try to leave the first query parameter intact | ||
| name = name.replace(/\?([^=]*)(=)?.*/, '?$1$2\u2026') |
lighthouse-core/lib/url-shim.js
Outdated
|
|
||
| // Remove it all if it's still too long | ||
| if (name.length > MAX_LENGTH) { | ||
| name = name.replace(/\?.*/, '?\u2026') |
There was a problem hiding this comment.
haha sorry you jumped the gun on my lint/tests commit :P (did a lot of personal work this weekend, and I use no semis)
lighthouse-core/lib/url-shim.js
Outdated
| name = parsed.pathname; | ||
| const parts = name.split('/'); | ||
| if (options.pathParts && parts.length > options.pathParts) { | ||
| name = '/\u2026/' + parts.slice(-1 * options.pathParts).join('/'); |
There was a problem hiding this comment.
i keep thinking pathParts is a split array. What about numParts?
There was a problem hiding this comment.
Do you think the part prefix is worth it? It takes up realestate that we otherwise could show more of the URL.
There was a problem hiding this comment.
I think it's confusing the way it's presented now and I use lighthouse everyday haha, so I'm in favor of displaying an ellipsis everywhere we remove characters. I'm cool with getting rid of the double/ if it's too much
There was a problem hiding this comment.
Yea. Remove the first '/' would be good. Otherwise, it looks like a relative URL, up a directory.
lighthouse-core/lib/url-shim.js
Outdated
| name = parsed.pathname; | ||
| const parts = name.split('/'); | ||
| if (options.numPathParts && parts.length > options.numPathParts) { | ||
| name = '\u2026' + parts.slice(-1 * options.numPathParts).join('/'); |
There was a problem hiding this comment.
nit: lots of \u2026 everywhere. Is that worth factoring out into a const var?
There was a problem hiding this comment.
yeah good call
| @@ -140,7 +140,7 @@ class UnusedCSSRules extends Audit { | |||
| const contentPreview = UnusedCSSRules.determineContentPreview(stylesheetInfo.content); | |||
| url = '*inline*```' + contentPreview + '```'; | |||
| } else { | |||
| url = URL.getDisplayName(url); | |||
| url = URL.getDisplayName(url, {removeQuery: false}); | |||
There was a problem hiding this comment.
false as an overriding option is kinda awkward. (just like on the CLI)
how about preserveQuery: true so we can flip these.
There was a problem hiding this comment.
good call, done
@ebidel brought up that a lot of the byte efficiency audits can look like buggy duplicates because the query strings are hidden. This PR adds options to
URL.getDisplayNamethatIn the examples below the duplicate dbw_tester is because of an XHR that actually requests it a second time.
Before
After
