Handle error messages from remote protocol correctly. #853
Conversation
If the result of parsing the incoming message from json => pojo contains an error, the `result` property will not exist. This means error handling later in the program will fail since you will not be able to access `error.code`, `error.stack` etc.
|
I'm going to break the build by merging this, but it exposes a runtime error that would have otherwise been caught when we merged https://github.com/GoogleChrome/lighthouse/pull/830/files#diff-5917acd1c4154d5b21b9c3c6ada674e6 |
| {method: callback.method, params: object.result}, 'error'); | ||
| callback.reject(object.result); | ||
| {method: callback.method}, 'error'); | ||
| callback.reject(object.error); |
There was a problem hiding this comment.
we might want to augment this message with information about it being from the debugging protocol (since I believe the log above that will only be on if you specify --verbose)
There was a problem hiding this comment.
Perhaps we should just new up an Error object? I can't vouch for the entire code base (yet) but it would be nice if any errors that flow through the promise pipeline were of a similar 'shape'
There was a problem hiding this comment.
... plus augment as you suggest
There was a problem hiding this comment.
I like the idea of both. Make the source of the error clear in the message (at the very least it'll make bug reports easier to track down) and standardize on reject(new Error(msg)) so that we can depend on the shape.
There was a problem hiding this comment.
Also by creating a new Error inline there, anyone can print the stack and at the very least get back to this protocol handling bit
SGTM |
If the result of parsing the incoming message from
json=>pojocontains an error, theresultproperty will not exist. This means error handling later in the program will fail sinceyou will not be able to access
error.code,error.stacketc.Before: (current master branch)
After:
