X Tutup
Skip to content

inspector: auto collect webstorage data#62145

Open
islandryu wants to merge 7 commits intonodejs:mainfrom
islandryu:autoCollectWebStorage
Open

inspector: auto collect webstorage data#62145
islandryu wants to merge 7 commits intonodejs:mainfrom
islandryu:autoCollectWebStorage

Conversation

@islandryu
Copy link
Member

@islandryu islandryu commented Mar 7, 2026

Web Storage data collection, which previously required explicitly firing events to send data to DevTools, is now performed automatically within Node.js.
Web Storage will appear in DevTools simply by specifying --experimental-storage-inspection, without requiring any code changes.

This feature relies on the changes introduced in the following change request, so it can be verified with the latest DevTools commit:
https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/7274801

@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 7, 2026
@codecov
Copy link

codecov bot commented Mar 7, 2026

Codecov Report

❌ Patch coverage is 93.25843% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.66%. Comparing base (a6e9e32) to head (295a5a4).
⚠️ Report is 67 commits behind head on main.

Files with missing lines Patch % Lines
src/inspector/dom_storage_agent.cc 81.57% 1 Missing and 6 partials ⚠️
src/node_webstorage.cc 77.27% 1 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62145      +/-   ##
==========================================
+ Coverage   89.64%   89.66%   +0.01%     
==========================================
  Files         676      677       +1     
  Lines      206240   206711     +471     
  Branches    39514    39582      +68     
==========================================
+ Hits       184891   185349     +458     
+ Misses      13465    13446      -19     
- Partials     7884     7916      +32     
Files with missing lines Coverage Δ
lib/internal/inspector/webstorage.js 100.00% <100.00%> (ø)
lib/internal/webstorage.js 100.00% <100.00%> (ø)
src/node_builtins.cc 75.95% <ø> (ø)
src/node_webstorage.h 83.33% <ø> (ø)
src/node_webstorage.cc 74.87% <77.27%> (+0.13%) ⬆️
src/inspector/dom_storage_agent.cc 67.56% <81.57%> (+17.88%) ⬆️

... and 71 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.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Looks good!

I'd still recommend adding a test for UTF-16 characters outside the ISO-8859-1 range (e.g. emoji or Chinese characters)

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2026
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Mar 8, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 8, 2026

Failed to start CI
- Validating Jenkins credentials
✔  Jenkins credentials valid
- Starting PR CI job
✘  Failed to start PR CI: 404 Not Found
https://github.com/nodejs/node/actions/runs/22820941170

@aduh95
Copy link
Contributor

aduh95 commented Mar 8, 2026

Related test failure:

Path: parallel/test-code-cache
Error: --- stderr ---
node:internal/bootstrap/realm:185
      mod = bindingObj[module] = getInternalBinding(module);
                                 ^

Error: No such binding: webstorage

You probably need to skip that test when compiled without SQLite

@islandryu islandryu force-pushed the autoCollectWebStorage branch from 8e0c41b to b278ecc Compare March 9, 2026 04:34
@islandryu
Copy link
Member Author

I changed the implementation to handle the storage keys and values as UTF-16.

@islandryu islandryu force-pushed the autoCollectWebStorage branch from b278ecc to 8fc09a7 Compare March 9, 2026 05:21
@nodejs-github-bot
Copy link
Collaborator

Co-authored-by: Anna Henningsen <github@addaleax.net>
@nodejs-github-bot
Copy link
Collaborator

@Renegade334 Renegade334 removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Mar 10, 2026
is_local_storage ? local_storage_map_ : session_storage_map_;
std::optional<StorageMap> storage_map =
is_local_storage ? std::make_optional<StorageMap>(local_storage_map_)
: std::make_optional<StorageMap>(session_storage_map_);
Copy link
Member

Choose a reason for hiding this comment

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

This is still making full copies – that's proably fine for a inspector feature, but it's an unnecessary set of extra allocations

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 10, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 10, 2026
@nodejs-github-bot
Copy link
Collaborator

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.

5 participants

X Tutup