fix: always ensure tracing is off before starting #2279
Conversation
| @@ -46,15 +46,16 @@ class Connection { | |||
| * Call protocol methods | |||
| * @param {!string} method | |||
| * @param {!Object} params | |||
| * @param {boolean=} silent | |||
There was a problem hiding this comment.
open to much better names, silenceError felt like it would supress the error itself, silenceErrorLogging too verbose?
| endTraceIfStarted() { | ||
| return new Promise((resolve) => { | ||
| const traceCallback = () => resolve(); | ||
| this.once('Tracing.tracingComplete', traceCallback); |
There was a problem hiding this comment.
why listen here? what about
this.sendCommand('Tracing.end').then(resolve, resolve);There was a problem hiding this comment.
nah it moves on too quickly and still fails fatally with tracing already started, this seemed preferable to an arbitrary timeout
lighthouse-core/gather/driver.js
Outdated
| * @return {!Promise} | ||
| */ | ||
| sendCommand(method, params) { | ||
| sendCommand(method, params, silent) { |
There was a problem hiding this comment.
im not crazy about the boolean trap here. do we need this at all if we follow my idea below?
There was a problem hiding this comment.
yeah regardless of that change the connection is currently hard coded for all messages with an error property to log them even if you don't care about the promise resolution
There was a problem hiding this comment.
the boolean trap is pretty gnarly. Before we hardcoded some error message to ignore it...we could do the same again. There could also be some kind of log trickery so that this call to sendCommand could silence an error it was expecting then turn the silencing off again. Pretty hacky ideas, though...
|
@paulirish @brendankenny switched away from bool PTAL :) |
| * @return {!Promise} | ||
| */ | ||
| sendCommand(method, params = {}) { | ||
| sendCommand(method, params = {}, options = {}) { |
There was a problem hiding this comment.
sg, but can rename to cmdOpts or something? so it's clear this is different from our beloved options friend?
There was a problem hiding this comment.
Should also document it, not just it takes some options :)
Wonder if it should be something like a regex so it's not just unconditionally ignoring errors. That would resemble the temporary fix we had for silencing domain enable/disable errors, which seemed like a good idea at the time
|
wdyt about |
I think it's a slightly better bandaid for the situation at hand but introduces unnecessary complexity for something we want to get rid of in forthcoming catharsis :) |
|
Wait, what are we removing that would fix the underlying problem? Or are you saying this is a temporary fix for something we want to actually fix? |
Oh that the connection class tries to do anything more than verbose logging to begin with! Isn't that what you were proposing anyway? |
|
let's wrap this guy up. i'm OK with merging and giving it a go.. see what happens. to you, @brendankenny |
fixes #2272