-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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(runner): rename generatedTime to fetchedAt #4783
Conversation
.then(_ => driver.getUserAgent()) | ||
.then(_ => { | ||
options.fetchedAt = (new Date()).toJSON(); | ||
return driver.getUserAgent(); |
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.
The other promise expects the user agent in return, throws else. Can convert to one-liner if you want to.
Looks like the Chrome Infra is using generatedTime, should I change that? |
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.
thanks for taking this on @ev1stensberg!
we'll have quite a bit to clean up at the same time when we roll LH to Chrome, so no need for any changes there yet. I've updated #4333 to make sure we don't lose track of this one :)
@@ -100,7 +100,10 @@ class GatherRunner { | |||
const resetStorage = !options.flags.disableStorageReset; | |||
// Enable emulation based on flags | |||
return driver.assertNoSameOriginServiceWorkerClients(options.url) | |||
.then(_ => driver.getUserAgent()) | |||
.then(_ => { | |||
options.fetchedAt = (new Date()).toJSON(); |
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.
think we can stick it on gathererResults instead like we do for user agent?
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.
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.
it makes a little more semantic sense to have it as an artifact like UserAgent as it is produced through the gathering process. also I think we'll have to to get this to work with the lighthouse -G && lighthouse -A
workflow that paul flagged as part of #4753 since we don't save options to disk, just artifacts
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.
it might need some updating of tests as you noticed though :) sorry!
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.
Makes sense now as you say it, a bit busy now, but going to address it during the weekend 👍
Also, noticed you're replacing Nexus in favor for Pixel in emulation. Could I take a crack at that? |
I think that proposal needs a little more discussion before we move to impl ;) love the enthusiasm though! |
@@ -348,6 +351,7 @@ class GatherRunner { | |||
// Nest LighthouseRunWarnings, if any, so they will be collected into artifact. | |||
const uniqueWarnings = Array.from(new Set(gathererResults.LighthouseRunWarnings)); | |||
gathererResults.LighthouseRunWarnings = [uniqueWarnings]; | |||
gathererResults.fetchedAt = [gathererResults.fetchedAt]; |
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.
Works now, sorry for delaying. Extremely hungover yesterday 😄
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.
Another thing, if people use this through the CLI, do we need to expose name etc in order to save it to disk using -A
or -G
?
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.
Haha ups, sorry about that!
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 for taking this on @ev1stensberg! 🎉 🏁
Fixes #4753
Summary
Refactored some code to fit the new convention.
fetchedAt
gets mounted to the options object and pulled in the runner. Default value/fallback is set in the formatter, so no need to set default value on an empty Date object (doesn't ever resolve to null either).Ping me if there's anything that needs to be revised.