Keep track of the document URL post-redirects #582
Conversation
redirects. `options.url` will be kept up-to-date, and `options.initialUrl` will retain the initial URL provided to Lighthouse
The drivers no longer keep track of the current URL.
obj. This is currently untested. Will need an end to end test to try redirects.
| .then(_ => { | ||
| // Update the outer options object of any post-redirect url | ||
| // Subsequent passes will use the new URL, and it'll be reported outwards | ||
| options.url = runOptions.url; |
There was a problem hiding this comment.
seems like this could bite us, though. Someone adding a new first pass, moving the performance gatherers to the second pass, would miss the redirects in the load time since the second pass would skip to the new URL. Is there a reason each pass shouldn't start with the same URL each time?
There was a problem hiding this comment.
Did it mostly to save time in our later passes, but I agree it changes behavior. I'm fine with reusing the same initialUrl for each pass
There was a problem hiding this comment.
Interesting challenge here. Thee http-redirect gather changes the options.url in beforePass and resets in afterPass.
They are separated into separate passes, so we are safe for now, but we need to make sure they don't share a pass. (well, until we fix the http redirect gather)
|
Rebased to master. To test, run |
paulirish
force-pushed
the
resolved-url
branch
from
365e89a
to
81e4d2f
Compare
| * This gatherer changes the options.url so that its pass loads the http page. | ||
| * After load it detects if its on a crypographic scheme. | ||
| * TODO: Instead of abusing a loadPage pass for this test, it could likely just do an XHR instead | ||
| */ | ||
| class HTTPRedirect extends Gatherer { |
There was a problem hiding this comment.
Actually wondering if we should be surfacing this. For example, redirecting to an m. incurs a perf penalty, which should be captured in TTI etc, but as a meta piece of data highlighting the number of redirects might be helpful.
There was a problem hiding this comment.
redirects do show up (but aren't especially highlighted) in the critical request chains though timing info isn't included
|
LGTM. Think we might want to surface the number of redirects as a piece of data somewhere, but that can be a separate PR. |
| * As such, options.url will always represent the post-redirected URL. | ||
| * options.initialUrl is the pre-redirect URL that things started with | ||
| * | ||
| * Caveat: only works when network recording enabled for a pass |
There was a problem hiding this comment.
should we file a bug for this? I'm thinking either network recording should always happen, getting rid of config.network, or network recording should always happen but the results are discarded if config.network is false
There was a problem hiding this comment.
seems important for gatherers/audits to be able to assume they don't have to do any work to get the actual url of the current page
|
It still feels a bit weird to be updating |
|
(we could explore that in a follow up with a general |
|
Yup I like the idea of doing it alongside the trace and network data. |
Often from a non-extension the input URL may then be redirected. For example, there are 3 redirects if you attempt
http://aliexpress.com:There's a few places where we work with the current URL, including @jeffposnick's new #578 and the unregsw branch. And in these cases, we want the post-redirect URL.
This PR will update
options.urlas redirects change the document location.Additionally, we used to have a
driver.url, but only for the chrome extension. We now drop that completely, which simplifies our URL gatherer.