Feat(Page): Add 'Log' domain entries #2400
Conversation
@yanivefraim from user perspective, these all are "console" messages, so I'd rather re-use the "console" event together with "ConsoleMessage" object. Thanks for checking in. |
@aslushnikov - agree, I will changed my implementation. One more question - when using
** and another comment - as far as I understand my |
|
I couldn't find a use case where we have |
There was a problem hiding this comment.
I couldn't find a use case where we have args on LogEntry. Any idea? (I want to add such a case for my test)
Looks like there's only one client who reports args - form autofill. We don't have autofill component in headless, so there's no good way to have a test here. However, if you manage to re-use the _onConsoleAPI logic, we'll be fine.
P.S. also check out cs.chromium.org: queries like -f:out f:inspector entryAdded can give valuable insights on inner workings of devtools protocol.
test/page.spec.js
Outdated
| }); | ||
| }); | ||
|
|
||
| describe('Page.Events.Log.entryAdded', function() { |
There was a problem hiding this comment.
let's move this to other console tests
There was a problem hiding this comment.
@aslushnikov - done
test/page.spec.js
Outdated
| await page.goto('about:blank'); | ||
| let message; | ||
| page.on('console', event => message = event); | ||
| page.evaluate(async() => fetch('https://example.org/').catch(e => {})); |
There was a problem hiding this comment.
nit: let's use server.EMPTY_PAGEinstead of example.com
lib/Page.js
Outdated
|
|
||
| _onLogEntryAdded(event) { | ||
| const {level, text, args} = event.entry; | ||
| this.emit(Page.Events.Console, new ConsoleMessage(level, text, args)); |
There was a problem hiding this comment.
args are RemoteObjects here; you'd need to convert these to JSHandles first.
It would be ideal if you can reuse all the logic that's already in page._onConsoleAPI code.
There was a problem hiding this comment.
@aslushnikov - I tried reusing page._onConsoleAPI but I found that it does not make much sense:
_onConsoleAPI initialize ConsoleMessage's text from args while Log.entryAdded take the text from event.text (when usually there are no args at all). My question is if we do need args for Log.entryAdded? I can refactor _onConsoleAPI to be compatible for both cases (taking text from args or from event.text), but because I don't have a test case for Log.entryAdded with args, I cannot really tell if this use case is working.
WDYT?
There was a problem hiding this comment.
@yanivefraim I see. Let's just disregard entryAdded arguments. You can release them like console does.
I also figured there's no executionContextId in the Log.entryAdded, so you won't be able to convert these args to JSHandles anyway.
There was a problem hiding this comment.
@aslushnikov - thanks - fixed (I hope that I understood you correct)
BTW, I noticed event.args.map(arg => helper.releaseObject(this._client, arg)); (in here) does not wait for promise to be resolved. I assume it is because we do not care about the response? (just making sure)
There was a problem hiding this comment.
BTW, I noticed event.args.map(arg => helper.releaseObject(this._client, arg)); (in here) does not wait for promise to be resolved. I assume it is because we do not care about the response? (just making sure)
That's right
WIP (I'm working on docs)Fixes #1939
Question: Do we want to return a LogEntry object, similar to
ConsoleMessage? (I guess the answer is yes, just making sure)