Skip to content

Comments

pass more websocket web-platform tests#6155

Merged
anonrig merged 1 commit intomainfrom
yagiz/fix-websockets-wpt
Feb 24, 2026
Merged

pass more websocket web-platform tests#6155
anonrig merged 1 commit intomainfrom
yagiz/fix-websockets-wpt

Conversation

@anonrig
Copy link
Member

@anonrig anonrig commented Feb 24, 2026

No description provided.

@anonrig anonrig requested a review from jasnell February 24, 2026 16:39
@anonrig anonrig requested review from a team as code owners February 24, 2026 16:39
Copy link
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

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

I'm Bonk, and I've done a quick review of your PR.

This PR moves WebSocket close() code/reason validation before readyState checks and changes the error type from TypeError to InvalidAccessError, fixing spec compliance and enabling more WPT tests to pass.

Issues

  1. [HIGH] Ungated behavioral change — error type and ordering. Two observable behaviors change here without a compat flag: (a) the exception type switches from TypeError to InvalidAccessError, and (b) validation now runs before readyState/state checks, meaning close(9999) on an already-closed socket will throw instead of silently returning. Both changes are spec-correct, but existing Workers that catch (e) { if (e instanceof TypeError) ... } or rely on the silent-return path will break. Per workerd's backward compat commitment, these should be gated behind a compatibility flag.

Note: these comments are generated by an AI assistant and may not be perfect. Please evaluate them critically.

@ask-bonk
Copy link
Contributor

ask-bonk bot commented Feb 24, 2026

I'm Bonk, and I've done a quick review of your PR.

This PR moves WebSocket close() code/reason validation before readyState checks and changes the error type from TypeError to InvalidAccessError, fixing spec compliance and enabling more WPT tests to pass.

Issue found (HIGH severity): The PR makes two ungated observable behavioral changes: (1) the exception type switches from TypeError to InvalidAccessError, and (2) validation now runs before readyState/state checks, meaning invalid-code calls on already-closed sockets will throw instead of silently returning. Both are spec-correct, but per workerd's backward compat commitment, they should be gated behind a compatibility flag.

Review posted to the PR with an inline comment on the affected code.

github run

Copy link
Contributor

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

See comment, otherwise LGTM

@codspeed-hq

This comment was marked as outdated.

@anonrig anonrig force-pushed the yagiz/fix-websockets-wpt branch from 0282c3d to 090e3bc Compare February 24, 2026 17:04
@anonrig anonrig requested a review from jasnell February 24, 2026 17:04
@anonrig anonrig force-pushed the yagiz/fix-websockets-wpt branch from 090e3bc to fe090df Compare February 24, 2026 17:37
@anonrig anonrig enabled auto-merge (squash) February 24, 2026 17:42
@anonrig anonrig merged commit de3f468 into main Feb 24, 2026
36 of 37 checks passed
@anonrig anonrig deleted the yagiz/fix-websockets-wpt branch February 24, 2026 19:08
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.

3 participants