`DDPDriver.connect` emits N `connected` events per open and resolves off its own emitter

Open
  • Is something wrong?
#281 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
52/100
Issue type
Refactor
Clarity
Mostly clear
Activity status
Quiet
Tech stack
typescript
Domain
api, networking

Research direction

First inspect Rocket.Chat.ReactNative for uses of the connected event and deprecated driver.subscriptions, since the SDK ships as source and the issue identifies those consumers as required checks. Then read lib/drivers/ddp.ts, especially the constructor, connect, connected, subscriptions, and waitForNotifyUserMediaSubs, along with ADR-0004. Done means one open echo, connect returning ddp.open(), and removal of the alias without breaking identified consumers.

Written by the indexing model from the issue text.

Description

Where: lib/drivers/ddp.ts, DDPDriver.connect and DDPDriver.connected.

Problem

There are three answers to "connected": Socket.connected, the Driver's connected event, and DDPDriver.connected.

connect registers a fresh open listener on the Socket every time it is called, with no dedupe — so N calls means N connected events per open. It then resolves by round-tripping through its own emitter rather than off open()'s promise, which makes "connect resolved" and "the Socket opened" two unrelated facts.

It also aliases this.subscriptions = this.ddp.subscriptions, feeding a field already marked @deprecated. That alias is the only reason ADR-0004 has to require forgetAllSubscriptions to mutate the map in place instead of replacing it.

Fix

  • Register the open echo once, in the constructor.
  • Have connect return ddp.open().
  • Delete DDPDriver.subscriptions and its alias, which removes the ADR-0004 constraint rather than working around it.

Needs checking first

Both the connected event and the deprecated driver.subscriptions field are reachable from Rocket.Chat.ReactNative. The SDK ships as source, so nothing in this repo compiles the consumer — someone has to read the app before removing either.

Follow-up in the same area

waitForNotifyUserMediaSubs is the biggest method in the Driver and reaches around its own interface: it reads this.ddp.subscriptions directly and calls this.ddp.subscribe rather than its own subscribe, because the sibling reshapes the args and drops the id. Then it poll-loops on a map it does not own — a second reader on the DDP subscription bookkeeping, which is what ADR-0004 exists to prevent.

Two ways out: give DDPDriver.subscribe an optional id so no bypass is needed, or move "resubscribe these named streams and await ready" onto the Socket where the map lives. Filed as a note here rather than its own issue because dropping the subscriptions alias above dissolves most of the ownership problem — worth re-reading after that lands.

Dominant language
TypeScript
Stars
146
Forks
101
No merged PRs in 30d

Getting set up

This project ships no dev container, Dockerfile or contributing guide, so setting up is up to you: start from its README, and see our first-contribution guide for the general steps.

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from RocketChat/Rocket.Chat.js.SDK

All issues in RocketChat/Rocket.Chat.js.SDK

Similar issues

More TypeScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.