-
Notifications
You must be signed in to change notification settings - Fork 10.1k
feat(socket.io): leave multiple rooms at once #5424
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
fc979d3 to
61ee20b
Compare
Changes SummaryThis PR extends the Socket.IO Type: feature Components Affected: socket.io-adapter, socket.io Architecture Impact
Risk Areas: Custom adapter implementations that extend the base Adapter class will need to update their del() method signature, The Socket.leave() documentation example changed from chaining (.leave('room1').leave('room2')) to array syntax, which is a behavioral change in recommended usage Suggestions
Full review in progress... | Powered by diffray |
| it("should leave multiple rooms at once", (done) => { | ||
| const io = new Server(0); | ||
| const client = createClient(io, "/"); | ||
|
|
||
| io.on("connection", (socket) => { | ||
| Promise.resolve(socket.join(["room1", "room2"])) | ||
| .then(() => Promise.resolve(socket.leave(["room1", "room2"]))) | ||
| .then(() => { | ||
| const adapter = io.of("/").adapter; | ||
| expect(adapter.rooms.has("room1")).to.be(false); | ||
| expect(adapter.rooms.has("room2")).to.be(false); | ||
| success(done, io, client); | ||
| }) | ||
| .catch(done); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 HIGH - Missing test coverage for single room parameter
Category: quality
Description:
The leave() method now accepts Room | Array<Room>, but the new test only covers array syntax. Single string usage is tested elsewhere in messaging-many.ts, but explicit coverage for backward compatibility would be valuable.
Suggestion:
Add a test case for leaving a single room using socket.leave("room1") syntax in the socket.ts test file to ensure backward compatibility is explicitly tested.
Confidence: 70%
Rule: test_new_parameter_coverage
Review Summary
Validated 10 issues: 4 kept (1 high test coverage gap, 1 high potential null reference, 2 medium code quality), 6 filtered (JSDoc nitpicks, false positive null assertions, minor test improvements) Issues Found: 4See 1 individual line comment(s) for details. 📊 2 unique issue type(s) across 4 location(s) 📋 Full issue list (click to expand)🟠 HIGH - Missing test coverage for single room parameterCategory: quality File: Description: The Suggestion: Add a test case for leaving a single room using Confidence: 70% Rule: 🟠 HIGH - Non-null assertion on optional property (3 occurrences)Category: bug 📍 View all locations
Rule: Powered by diffray - Multi-Agent Code Review Agent |
The kind of change this PR does introduce
Current behavior
Can only leave single room with Socket.leave() method
New behavior
Server-side sockets can now leave several rooms in a single socket.leave([...]) call. The in-memory adapter (and tests) were updated so Adapter.del() accepts a Set, keeping server state consistent without per-room calls.
Other information (e.g. related issues)
#5391