-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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(main-resource): adjust main resource identification logic #4475
Conversation
@patrickhulce @paulirish PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @kdzwinel I had my review sitting here and never hit submit!! Thoughts below
I think ideally we'd have a single source of truth for the final URL rather than duplicate copies of the logic, even if it is pretty small
Which of the following sounds best to folks
- Change the computed artifact to accept the final URL/requestId as input, and find the correct network record that way
- Change the driver code to just call this function to get the final URL instead of keeping track as we go
- Add an assert to all usages of requestMainResource that it matches the URL artifact
@patrickhulce no worries! Reusing |
@@ -16,7 +16,7 @@ module.exports = { | |||
return Promise.resolve(); | |||
}, | |||
gotoURL() { | |||
return Promise.resolve('https://example.com'); | |||
return Promise.resolve('https://www.reddit.com/r/nba'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's the correct URL for the ../fixtures/traces/progressive-app.json
returned by the fake-driver
@@ -330,6 +330,9 @@ describe('Runner', () => { | |||
], | |||
|
|||
artifacts: { | |||
URL: { | |||
finalUrl: 'https://www.reddit.com/r/nba', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
critical-request-chains now depends on URL via mainResource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks @kdzwinel!
the appveyor failure is hopefully unrelated @kdzwinel ? |
@patrickhulce fun bug alert! Hm… It fails on AppVeyor but passes on Travis? Yesterday I couldn't reproduce it locally, but today I can?? It fails on master??? Mystery! So I started debugging and it looks like DevTools in Chrome 63 (stable): |
awesome sleuthing and thanks for filing the crbug @kdzwinel! |
this PR aligns main-resource gatherer logic with final URL logic from driver.js
lighthouse/lighthouse-core/gather/driver.js
Lines 611 to 622 in 3030b4f
Fixes #4454
Fixes #4440