Skip to content

inspector: return errors when CDP protocol event emission fails#62162

Open
islandryu wants to merge 1 commit intonodejs:mainfrom
islandryu:returnError
Open

inspector: return errors when CDP protocol event emission fails#62162
islandryu wants to merge 1 commit intonodejs:mainfrom
islandryu:returnError

Conversation

@islandryu
Copy link
Member

@islandryu islandryu commented Mar 9, 2026

Previously, there was no feedback when functions that emit events such as Network.webSocketCreated failed. 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.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/inspector

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 9, 2026
@codecov
Copy link

codecov bot commented Mar 9, 2026

Codecov Report

❌ Patch coverage is 80.55556% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.70%. Comparing base (3725bd2) to head (7c62581).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
src/inspector/dom_storage_agent.cc 26.92% 19 Missing ⚠️
src/inspector/network_agent.cc 96.96% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62162      +/-   ##
==========================================
+ Coverage   89.65%   89.70%   +0.04%     
==========================================
  Files         676      676              
  Lines      206543   206640      +97     
  Branches    39547    39550       +3     
==========================================
+ Hits       185184   185366     +182     
+ Misses      13480    13465      -15     
+ Partials     7879     7809      -70     
Files with missing lines Coverage Δ
lib/inspector.js 97.22% <100.00%> (ø)
src/inspector_agent.cc 80.74% <100.00%> (+0.22%) ⬆️
src/inspector_agent.h 100.00% <ø> (ø)
src/inspector_js_api.cc 85.15% <100.00%> (+0.10%) ⬆️
src/inspector/network_agent.cc 88.03% <96.96%> (+26.35%) ⬆️
src/inspector/dom_storage_agent.cc 47.54% <26.92%> (-2.15%) ⬇️

... and 47 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Isolate* isolate = Isolate::GetCurrent();
if (!ObjectGetProtocolString(context, storage_id_obj, "securityOrigin")
.To(&security_origin)) {
ThrowEventError(isolate, "Missing securityOrigin in storageId");
Copy link
Member

@legendecas legendecas Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

-->

* `params` {Object}
* Returns: {Array} An array of errors from each session
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preferrably, an array of errors should be thrown as AggregateError.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants