fix: warn about redirected urls #1605
Conversation
| @@ -281,6 +281,14 @@ function runLighthouse(url: string, | |||
| return results; | |||
| }) | |||
| .then((results: Results) => { | |||
| if (url !== results.url) { | |||
There was a problem hiding this comment.
should we move this patch to runner.js so our other distributions get the log as well?
There was a problem hiding this comment.
How are logs surfaced there? I really think it's unlikely to have any impact unless it's prominent. Come to think of it, thoughts on injecting the warning into the report itself?
There was a problem hiding this comment.
Come to think of it, thoughts on injecting the warning into the report itself?
#1512 kind of captures the need for this. For stuff that's logged anyway, one easy fix is to capture any warning or error log messages and somehow display them in the report.
There was a problem hiding this comment.
after #1677 I now realize you should do the comparison with URL.equalWithExcludedFragments so it doesn't warn on dropped fragments
There was a problem hiding this comment.
actually, now I wonder if that's right :) Excluding the fragment makes sense for comparing against network records, but for sites doing some state based stuff with the fragment maybe we should warn that you ended up in a different state than you intended (at least according to the URL)
wdyt?
|
Just bumping this, @brendankenny do you think we should hold off until we fully resolve #1512? |
| @@ -281,6 +281,14 @@ function runLighthouse(url: string, | |||
| return results; | |||
| }) | |||
| .then((results: Results) => { | |||
| if (url !== results.url) { | |||
There was a problem hiding this comment.
after #1677 I now realize you should do the comparison with URL.equalWithExcludedFragments so it doesn't warn on dropped fragments
I feel like this is less effective while we figure out #1512 but still helpful, but agree with @paulirish that this makes a lot more sense inside lighthouse instead of in the CLI. |
|
I'm going to close this in favor of starting work towards consistent error/warning reporting and surfacing. |
fixes #715
How it looks.
I stuck it after everything since putting at the top likely means no one will know.