feat(page): exposed additional info for ConsoleMessage object #3365
Conversation
| @@ -179,11 +179,11 @@ class Page extends EventEmitter { | |||
| * @param {!Protocol.Log.entryAddedPayload} event | |||
| */ | |||
| _onLogEntryAdded(event) { | |||
There was a problem hiding this comment.
This is not the only entry point for console messages. There is also Runtime.consoleAPICalled. It has line number and url contained in the stack trace.
There was a problem hiding this comment.
can you please elaborate?
I'm looking into Runtime.consoleAPICalled and I don't see such details, nor a reference to ExceptionDetails
There was a problem hiding this comment.
It is contained in the callframes of the stacktrace: https://chromedevtools.github.io/devtools-protocol/tot/Runtime#type-CallFrame
docs/api.md
Outdated
| @@ -2168,6 +2169,11 @@ puppeteer.launch().then(async browser => { | |||
|
|
|||
| [ConsoleMessage] objects are dispatched by page via the ['console'](#event-console) event. | |||
|
|
|||
| #### consoleMessage.additionalInfo() | |||
There was a problem hiding this comment.
Why not just put the url and line number on the console message itself?
There was a problem hiding this comment.
you mean extending the api? same as type and args?
There was a problem hiding this comment.
as I commented in the origin issue:
This is under the assumption that additionalInfo is a subset of LogEntry optional params, thus future requests for additional data can easily added
makes sense?
There was a problem hiding this comment.
also the fact the lineNumber and url are optionals, made me feel less comfortable in adding a dedicated and explicit API (as it will return undefined many times...)
There was a problem hiding this comment.
Having additionalInfo raises some difficult questions. Which values are considered core, and which are considered additional? Can additional info be null, or will it be an empty object? Even if we have reasoning, its not obvious to a user of the API. They would have to check the documentation every time.
That being said, it might make sense to group line, column, and url under a location object if they are often all null or all present.
There was a problem hiding this comment.
done. location it is.
tompere
force-pushed
the
fix-3029
branch
4 times, most recently
from
d283916
to
05878a3
Compare
This patch add additional method to ConsoleMessage object sent on console event. Currently, added URL and line number to additionalInfo. Fixes puppeteer#3029
lib/Page.js
Outdated
| let location; | ||
| if (event.stackTrace && event.stackTrace.callFrames) { | ||
| const {url, lineNumber, columnNumber} = event.stackTrace.callFrames[0]; | ||
| location = {url, lineNumber, columnNumber}; |
There was a problem hiding this comment.
nit: don't unconditionally add the properties if they might be undefined. Otherwise a user doing "url" in location will be misled.
There was a problem hiding this comment.
just a clarification about how defensive puppeteer code should be:
Can i rely on devtools protocol API for optional/mandatory return values?
e.g.
- Runtime#type-CallFrame always return
{url, lineNumber, columnNumber}. - Runtime#event-consoleAPICalled might not return
StackTrace. But if returned, it will always returnArray{CallFrame}.
There was a problem hiding this comment.
You can rely on the values that the protocol will provide, but there might be extra values alongside those values. So you should build your own object instead of serving the raw protocol object.
| @@ -517,7 +522,7 @@ class Page extends EventEmitter { | |||
| * @param {string} type | |||
| * @param {!Array<!Puppeteer.JSHandle>} args | |||
| */ | |||
| _addConsoleMessage(type, args) { | |||
| _addConsoleMessage(type, args, location) { | |||
This patch add additional method to ConsoleMessage object sent on console event. Currently, added URL and line number to additionalInfo. Fixes puppeteer#3029
JoelEinbinder
changed the title
|
👏 |
This patch add additional method to ConsoleMessage object sent on console event. Currently, added URL and line number to additionalInfo.
Fixes #3029