Skip to content

webstreams: allow shared array buffer sources in writable adapter#62163

Open
Renegade334 wants to merge 1 commit intonodejs:mainfrom
Renegade334:webstream-adapter-writable-buffer-sources
Open

webstreams: allow shared array buffer sources in writable adapter#62163
Renegade334 wants to merge 1 commit intonodejs:mainfrom
Renegade334:webstream-adapter-writable-buffer-sources

Conversation

@Renegade334
Copy link
Copy Markdown
Member

The webidl buffer source definition includes array buffers as well as array buffer views. Since node streams don't handle the former, #61913 added a transform step to the Writable.toWeb adapter to accept ArrayBuffers.

This PR expands the adapter behaviour to include SharedArrayBuffers – since the adapter already accepts views on shared array buffers, it doesn't make much sense to exclude these asymmetrically from the adapter logic.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Mar 9, 2026
Comment on lines +235 to +237
if (!streamWritable.writableObjectMode && isAnyArrayBuffer(chunk)) {
chunk = new Uint8Array(chunk);
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This operation can throw (if chunk is a detached array buffer), so should be within the try/catch block.

@Renegade334 Renegade334 force-pushed the webstream-adapter-writable-buffer-sources branch from ad1a533 to 2d3e275 Compare March 9, 2026 12:18
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 9, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.78%. Comparing base (ae1c0e4) to head (bb853fc).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62163      +/-   ##
==========================================
- Coverage   89.79%   89.78%   -0.02%     
==========================================
  Files         697      697              
  Lines      215773   215773              
  Branches    41297    41305       +8     
==========================================
- Hits       193749   193724      -25     
- Misses      14117    14157      +40     
+ Partials     7907     7892      -15     
Files with missing lines Coverage Δ
lib/internal/webstreams/adapters.js 86.44% <100.00%> (ø)
lib/internal/webstreams/compression.js 100.00% <100.00%> (ø)

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

@Renegade334 Renegade334 requested a review from panva April 8, 2026 10:20
@panva
Copy link
Copy Markdown
Member

panva commented Apr 8, 2026

@Renegade334 how is this aligned with the spec and #62632?

@Renegade334
Copy link
Copy Markdown
Member Author

Renegade334 commented Apr 8, 2026

This doesn't affect CompressionStream, just the Writable.toWeb adapter.

We currently have the scenario where the adapter will silently transform ArrayBuffer to Uint8Array before passing to the node stream (since ArrayBuffer is a valid buffer source in the web spec), but SharedArrayBuffers are not transformed and instead rejected – whereas node streams obviously accept views on SharedArrayBuffers, so there's unnecessary asymmetry.

CompressionStream correctly rejects both SABs and views on SABs, and this doesn't change.

@panva
Copy link
Copy Markdown
Member

panva commented Apr 8, 2026

Ok, can you rebase your PR and -s the commit?

Refs: nodejs#61913
Signed-off-by: Renegade334 <contact.9a5d6388@renegade334.me.uk>
@Renegade334 Renegade334 force-pushed the webstream-adapter-writable-buffer-sources branch from 2d3e275 to bb853fc Compare April 8, 2026 10:47
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

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

Labels

needs-ci PRs that need a full CI run. web streams

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants