Skip to content

Allow empty protocols array in WebSocket constructor#5963

Merged
anonrig merged 2 commits intomainfrom
yagiz/fix-websocket-empty-protocols
Jan 27, 2026
Merged

Allow empty protocols array in WebSocket constructor#5963
anonrig merged 2 commits intomainfrom
yagiz/fix-websocket-empty-protocols

Conversation

@anonrig
Copy link
Copy Markdown
Member

@anonrig anonrig commented Jan 27, 2026

Fixes #5822

Per the WebSocket spec, an empty protocols array is valid and equivalent to not specifying any protocols. The spec uses [] as the default value for the optional protocols parameter.

Previously, workerd threw a SyntaxError when an empty array was passed. Now we simply skip setting the Sec-WebSocket-Protocol header in this case.

@anonrig anonrig requested review from a team as code owners January 27, 2026 14:27
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Jan 27, 2026

Merging this PR will degrade performance by 12.13%

❌ 1 regressed benchmark
✅ 69 untouched benchmarks
⏩ 129 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
simpleStringBody[Response] 19 µs 21.6 µs -12.13%

Comparing yagiz/fix-websocket-empty-protocols (32aad24) with main (727c2e4)

Open in CodSpeed

Footnotes

  1. 129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 27, 2026

Codecov Report

❌ Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.14%. Comparing base (a0746ee) to head (32aad24).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
src/workerd/api/web-socket.c++ 88.23% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5963      +/-   ##
==========================================
- Coverage   70.14%   70.14%   -0.01%     
==========================================
  Files         407      407              
  Lines      107188   107190       +2     
  Branches    17965    17966       +1     
==========================================
- Hits        75192    75191       -1     
- Misses      21209    21214       +5     
+ Partials    10787    10785       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 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
Copy Markdown
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

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

LGTM with a couple nits.

Co-authored-by: Victor Berchet <victor@suumit.com>
@anonrig anonrig enabled auto-merge (squash) January 27, 2026 15:44
@anonrig anonrig requested review from jasnell and npaun January 27, 2026 16:40
@anonrig anonrig merged commit f107510 into main Jan 27, 2026
22 checks passed
@anonrig anonrig deleted the yagiz/fix-websocket-empty-protocols branch January 27, 2026 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐛 Bug Report — WebSocket constructor throws SyntaxError error when protocols array is empty

4 participants