inspector: return errors when CDP protocol event emission fails#62162
inspector: return errors when CDP protocol event emission fails#62162islandryu wants to merge 2 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62162 +/- ##
==========================================
+ Coverage 89.65% 89.73% +0.07%
==========================================
Files 676 676
Lines 206543 206666 +123
Branches 39547 39559 +12
==========================================
+ Hits 185184 185448 +264
+ Misses 13480 13386 -94
+ Partials 7879 7832 -47
🚀 New features to boost your workflow:
|
| Isolate* isolate = Isolate::GetCurrent(); | ||
| if (!ObjectGetProtocolString(context, storage_id_obj, "securityOrigin") | ||
| .To(&security_origin)) { | ||
| ThrowEventError(isolate, "Missing securityOrigin in storageId"); |
There was a problem hiding this comment.
IIUC, this would throw an identical error for each session connected. Can we validate the object shape once so that the error is only thrown once? This would also prevent returning an array of errors in the JS API.
There was a problem hiding this comment.
If errors that depend on session-specific state are introduced in the future, this may need to be changed. However, since we currently only perform type checks, would it make sense to throw the error directly?
I’ve added a commit to implement this change.
doc/api/inspector.md
Outdated
| --> | ||
|
|
||
| * `params` {Object} | ||
| * Returns: {Array} An array of errors from each session |
There was a problem hiding this comment.
Preferrably, an array of errors should be thrown as AggregateError.
Previously, there was no feedback when functions that emit events such as
Network.webSocketCreatedfailed. This change modifies the behavior so that feedback can be obtained when such failures occur.Since an event may be emitted to multiple sessions, instead of throwing an error directly, the errors are returned as an array.