-
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(artifacts): create a PublicGathererArtifacts type #8382
Conversation
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.
lg!
@@ -12,7 +12,7 @@ | |||
|
|||
const Gatherer = require('./gatherer'); | |||
|
|||
class ChromeConsoleMessages extends Gatherer { | |||
class ConsoleMessages extends Gatherer { |
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 :)
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!
(looks like there are a few more that are simple/stable that we can move over, but we aren't in a rush) |
Summary
In conjuction with #8381, this moves us to blessed artifacts. The only non-typing change is to
ChromeConsoleMessages
which just removes theChrome
prefix. IMO, that feels more like the public API we might want to expose, but it's a trivial change if we want to revert.Our friends at adspeed only use
ViewportDimensions
,IFrameElements
,devtoolsLogs
, andtraces
. We haven't createdIFrameElements
yet but we can always increase the scope of our blessed artifacts later.After discussion with @brendankenny and @paulirish, we'll probably rely on documentation to prevent folks from venturing too far on their own with computed artifacts.
Related Issues/PRs
#8381