Skip to content
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(lhr): s/initialUrl/requestedUrl, s/url/finalUrl #5127

Merged
merged 4 commits into from
May 6, 2018

Conversation

brendankenny
Copy link
Member

also removes the URL gatherer and collects the {requestedUrl, finalUrl} artifact in GatherRunner itself (alongside fetchTime, UserAgent, etc). It will now always be collected (which makes sense).

This allows us to finally break the mutable options.url from outside Runner.run all the way down into every gatherer :):) passContext.url is still mutable and follows the same behavior as before (to allow things like the http-redirect gatherer to work), but the connection is broken outside of that except for the single explicit write to the artifact.

@paulirish paulirish mentioned this pull request May 4, 2018
82 tasks
@@ -119,12 +119,14 @@ class Runner {
reportCategories = ReportScoring.scoreAllCategories(opts.config.categories, resultsById);
}

const finalUrl = artifacts.URL ? artifacts.URL.finalUrl : '';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when do we not have one? isn't it always set now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol i'm reading the opposite. since this PR nukes the URL gatherer how is artifacts.URL set?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jk found it. it does appear to be set always..
so is this just handling someone has older saved artifacts they are -A'ing? lil funky.

Copy link
Collaborator

@patrickhulce patrickhulce May 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well he manually hardcodes artifacts.URL like we do run warnings/useragent/fetchTime so seems like it's a given now, but maybe I'm reading it wrong...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. Left over from getting un-updated tests to run :)

only future issue for those of us using saved artifacts (aka @paulirish) is that this is a breaking saved artifacts change, and since it'll be "cannot read property 'finalUrl' of undefined", this will throw on any -A run with previously saved artifacts instead of producing an LHR with an empty finalUrl

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup! no worries. :)

@@ -1 +1,6 @@
{}
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty-ish-artifacts

@@ -88,7 +88,7 @@ describe('Module Tests', function() {
});

it('should return formatted LHR when given no categories', function() {
const exampleUrl = 'https://example.com/';
const exampleUrl = 'https://www.reddit.com/r/nba';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, any particular reason?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think thats what the devtoolsLog was for those fixtures, which spread to the artifacts, and now to the input URL (though I guess technically only the finalUrl assertion would have to change :)

@@ -414,6 +414,7 @@ class GatherRunner {
const gathererResults = {
LighthouseRunWarnings: [],
fetchTime: [(new Date()).toJSON()],
URL: [{requestedUrl: options.requestedUrl, finalUrl: ''}],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh there it is.

const passContext = Object.assign({}, options, {passConfig});
const passContext = {
driver: options.driver,
// If the main document redirects, we'll update this to keep track
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ace. thx for leaving this.

@@ -119,12 +119,14 @@ class Runner {
reportCategories = ReportScoring.scoreAllCategories(opts.config.categories, resultsById);
}

const finalUrl = artifacts.URL ? artifacts.URL.finalUrl : '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jk found it. it does appear to be set always..
so is this just handling someone has older saved artifacts they are -A'ing? lil funky.

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants