-
Notifications
You must be signed in to change notification settings - Fork 13.8k
chore(@rocket.chat/ddp-client): introduce experimental Meteor independent DDP client (re-add) #40430
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
chore(@rocket.chat/ddp-client): introduce experimental Meteor independent DDP client (re-add) #40430
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@rocket.chat/meteor': minor | ||
| --- | ||
|
|
||
| Adds a new admin setting `Use_RC_SDK` (General → Use Rocket.Chat SDK) that opts the workspace into the experimental SDK-over-DDP transport. When enabled, the client routes Meteor DDP traffic through `@rocket.chat/ddp-client` over a single WebSocket instead of the legacy Meteor stream. The flag is dormant by default; the server surfaces the value via a `<meta name="rc-sdk-transport-enabled">` tag, and the client also honors a per-tab `?sdk_transport=on|off` URL parameter and a `rc-config-sdk_transport` localStorage key (URL > localStorage > meta tag). |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -50,7 +50,11 @@ export abstract class CachedStore<T extends IRocketChatRecord, U = T> implements | |||||||||||
|
|
||||||||||||
| protected eventType: StreamNames; | ||||||||||||
|
|
||||||||||||
| private readonly version = 18; | ||||||||||||
| // Bumped from 18 → 19 to invalidate caches populated before the DDPSDK | ||||||||||||
| // wire encoding was switched from JSON to EJSON. Entries written by the | ||||||||||||
| // JSON window stored dates as ISO strings instead of Date instances, so | ||||||||||||
| // fields like subscription.ls would fail `.getTime()` when read back. | ||||||||||||
|
Comment on lines
+53
to
+56
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove inline implementation commentary in this block. The new multi-line rationale comment should be moved to PR/changeset docs, keeping implementation comment-free per repo rule. Proposed change- // Bumped from 18 → 19 to invalidate caches populated before the DDPSDK
- // wire encoding was switched from JSON to EJSON. Entries written by the
- // JSON window stored dates as ISO strings instead of Date instances, so
- // fields like subscription.ls would fail `.getTime()` when read back.
private readonly version = 19;As per coding guidelines, 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||
| private readonly version = 19; | ||||||||||||
|
|
||||||||||||
| private updatedAt = new Date(0); | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,61 @@ | ||
| import { Accounts } from 'meteor/accounts-base'; | ||
|
|
||
| import { getUserId } from './user'; | ||
| import { isSdkTransportEnabled } from './sdk/sdkTransportEnabled'; | ||
| import { getUserId, userIdStore } from './user'; | ||
|
|
||
| const sdkTransportEnabled = isSdkTransportEnabled(); | ||
|
|
||
| const isLoggedIn = () => { | ||
| const uid = getUserId(); | ||
| return !!uid; | ||
| }; | ||
|
|
||
| /** | ||
| * Fire `cb` whenever the local userId transitions from absent → present. | ||
| * | ||
| * `Accounts.onLogin` would normally cover this, but Meteor only invokes | ||
| * the onLogin hook from inside a Tracker.autorun that waits for | ||
| * `Meteor.userAsync()` to resolve to a real user doc. When a login goes | ||
| * through our REST fallback (e.g. logout → fresh login while DDPSDK is | ||
| * reconnecting), the user document never lands in Meteor.users — it | ||
| * normally arrives as a DDP collection frame, but the REST endpoint | ||
| * only returns the method result. The autorun then sees a null user | ||
| * forever, and onLogin never fires. By piggybacking on userIdStore (which | ||
| * is updated synchronously the moment Accounts.connection.userId() is | ||
| * set), we get a reliable login signal regardless of how the user doc | ||
| * eventually arrives. | ||
| */ | ||
| const subscribeToLogin = (handler: () => void): (() => void) => { | ||
| let lastSeen = userIdStore.getState(); | ||
| return userIdStore.subscribe((next) => { | ||
| if (next === lastSeen) return; | ||
| const wasLoggedOut = !lastSeen; | ||
| lastSeen = next; | ||
| if (next && wasLoggedOut) { | ||
| handler(); | ||
| } | ||
| }); | ||
| }; | ||
|
|
||
| export const whenLoggedIn = () => { | ||
| if (isLoggedIn()) { | ||
| return Promise.resolve(); | ||
| } | ||
|
|
||
| if (!sdkTransportEnabled) { | ||
| // Flag off: develop's exact implementation — wait on Accounts.onLogin only, | ||
| // no userIdStore bridge. | ||
| return new Promise<void>((resolve) => { | ||
| const subscription = Accounts.onLogin(() => { | ||
| subscription.stop(); | ||
| resolve(); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| return new Promise<void>((resolve) => { | ||
| const subscription = Accounts.onLogin(() => { | ||
| subscription.stop(); | ||
| const stop = subscribeToLogin(() => { | ||
| stop(); | ||
| resolve(); | ||
| }); | ||
| }); | ||
|
|
@@ -30,11 +71,18 @@ export const onLoggedIn = (cb: (() => () => void) | (() => Promise<() => void>) | |
| } | ||
| }; | ||
|
|
||
| const subscription = Accounts.onLogin(handler); | ||
| // With the SDK transport on, login can land via REST (ddpOverREST) without | ||
| // filling Meteor.users — Accounts.onLogin's autorun would never fire. | ||
| // Bridge off userIdStore as belt-and-braces. With the flag off, the legacy | ||
| // DDP path populates Meteor.users and Accounts.onLogin fires reliably; the | ||
| // extra userIdStore subscription would just double-fire callbacks. | ||
| const accountsSubscription = Accounts.onLogin(handler); | ||
| const stopUserIdSubscription = sdkTransportEnabled ? subscribeToLogin(handler) : undefined; | ||
| if (isLoggedIn()) handler(); | ||
|
Comment on lines
+74
to
81
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
With SDK transport enabled, Line 79 and Line 80 subscribe to two different sources for the same login transition. This PR already had to add 🤖 Prompt for AI Agents |
||
|
|
||
| return () => { | ||
| subscription.stop(); | ||
| accountsSubscription.stop(); | ||
| stopUserIdSubscription?.(); | ||
| cleanup?.(); | ||
| }; | ||
| }; | ||
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.
ready()can hang indefinitely ifensureConnectedAndAuthenticated()never settles.The auth gate is wrapped in
.catch(() => undefined).then(...), so a rejected auth still subscribes. But a pending auth (e.g., DDP socket never reachesconnectedbecause there's no timeout inwaitForConnected) means the.thencallback never runs, nosubscriptionis created, and any caller awaitingstream.ready()waits forever — includinguseReactiveValueconsumers that gate UI on subscription readiness.Consider either:
ee.emit('ready', [err])soready()rejects, orsdk.connection.status === 'connected' && sdk.account.uid(most post-login subscription calls), and only deferring on the explicitly-anonymous path.🤖 Prompt for AI Agents