-
Notifications
You must be signed in to change notification settings - Fork 13.9k
refactor(client): drop Tracker.autorun from 3 easy autorun bridges #40445
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
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': patch | ||
| --- | ||
|
|
||
| Drop Meteor `Tracker.autorun` from three client call sites in favour of direct event subscriptions. `meteorBackedSdk`'s connection-status bridge now listens on `Meteor.connection._stream`'s low-level `'connected'`/`'disconnect'`/`'reset'` events instead of riding `Meteor.status()`'s reactive layer; `CachedStore`'s post-reconnect cache sync now hooks `sdk.connection.on('connection', …)` directly; `createComposerAPI`'s formatting-button watcher subscribes to `settings.observe('*', …)`. Net behaviour is unchanged — same recompute / re-sync triggers — but the reactivity travels through the SDK and the settings store rather than Tracker, removing 3 of the 10 client `meteor/tracker` imports. |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,12 +21,14 @@ interface StreamerOptions { | |||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| interface StreamerDDPConnection { | ||||||||||||||
| _stream: { | ||||||||||||||
| on: { | ||||||||||||||
| (key: 'message', callback: (data: string) => void): void; | ||||||||||||||
| (key: 'reset', callback: () => void): void; | ||||||||||||||
| }; | ||||||||||||||
| }; | ||||||||||||||
| _stream: | ||||||||||||||
| | { | ||||||||||||||
| on: { | ||||||||||||||
| (key: 'message', callback: (data: string) => void): void; | ||||||||||||||
| (key: 'reset', callback: () => void): void; | ||||||||||||||
| }; | ||||||||||||||
| } | ||||||||||||||
| | undefined; | ||||||||||||||
| subscribe(name: string, ...args: unknown[]): SubscriptionHandle; | ||||||||||||||
| call(methodName: string, ...args: unknown[]): void; | ||||||||||||||
| hasMeteorStreamerEventListeners?: boolean; | ||||||||||||||
|
|
@@ -46,7 +48,7 @@ export class StreamerCentral extends EV { | |||||||||||||
| return; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| ddpConnection._stream.on('message', (rawMessage?: unknown) => { | ||||||||||||||
| ddpConnection._stream!.on('message', (rawMessage?: unknown) => { | ||||||||||||||
|
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. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify all non-null assertions on optional Meteor streams to audit crash paths.
rg -nP --type=ts -C2 '\._stream!\.on\('Repository: RocketChat/Rocket.Chat Length of output: 1300 🏁 Script executed: # First, examine the StreamerDDPConnection class definition and _stream field
cat -n apps/meteor/client/lib/streamer/streamer.ts | head -60Repository: RocketChat/Rocket.Chat Length of output: 2098 🏁 Script executed: # Get more context around the problematic lines to understand initialization
cat -n apps/meteor/client/lib/streamer/streamer.ts | sed -n '20,130p'Repository: RocketChat/Rocket.Chat Length of output: 3838 🏁 Script executed: # Check the type definition of _stream - search for where it's declared
rg -n "_stream" apps/meteor/client/lib/streamer/streamer.ts -B2 -A2Repository: RocketChat/Rocket.Chat Length of output: 435 🏁 Script executed: # Check if StreamerDDPConnection extends or uses Meteor's DDP connection
rg -n "class StreamerDDPConnection\|extends\|implements" apps/meteor/client/lib/streamer/streamer.ts | head -20Repository: RocketChat/Rocket.Chat Length of output: 48 Guard
Suggested fix- ddpConnection._stream!.on('message', (rawMessage?: unknown) => {
+ const stream = ddpConnection._stream;
+ if (!stream) {
+ return;
+ }
+ stream.on('message', (rawMessage?: unknown) => {- this.ddpConnection._stream!.on('reset', () => {
+ const stream = this.ddpConnection._stream;
+ if (!stream) {
+ return;
+ }
+ stream.on('reset', () => {Applies to lines 51 and 112. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| if (typeof rawMessage !== 'string') { | ||||||||||||||
| return; | ||||||||||||||
| } | ||||||||||||||
|
|
@@ -75,7 +77,7 @@ export class StreamerCentral extends EV { | |||||||||||||
| getStreamer<N extends EventNames>(name: N, options: StreamerOptions): Streamer<N> { | ||||||||||||||
| const existingInstance = this.instances[name]; | ||||||||||||||
| if (existingInstance) { | ||||||||||||||
| return existingInstance as Streamer<N>; | ||||||||||||||
| return existingInstance; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| const streamer = new Streamer(name, options); | ||||||||||||||
|
|
@@ -107,7 +109,7 @@ export class Streamer<N extends EventNames> extends EV { | |||||||||||||
| this.name = name; | ||||||||||||||
| this.useCollection = useCollection; | ||||||||||||||
|
|
||||||||||||||
| this.ddpConnection._stream.on('reset', () => { | ||||||||||||||
| this.ddpConnection._stream!.on('reset', () => { | ||||||||||||||
| super.emit('__reconnect__'); | ||||||||||||||
| }); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,42 +26,14 @@ import { isSdkTransportEnabled } from '../../lib/sdk/sdkTransportEnabled'; | |
| * - connect/pong frames — discarded; the SDK socket has its own handshake. | ||
| */ | ||
|
|
||
| type MeteorIDDPStream = { | ||
| currentStatus: { | ||
| status: string; | ||
| connected: boolean; | ||
| retryCount: number; | ||
| retryTime?: number; | ||
| reason?: string; | ||
| }; | ||
| eventCallbacks?: Record<string, Array<(...args: unknown[]) => void>>; | ||
| statusListeners?: { changed(): void }; | ||
| on(event: string, callback: (...args: unknown[]) => void): void; | ||
| forEachCallback(name: string, cb: (callback: (...args: unknown[]) => void) => void): void; | ||
| send(data: string): void; | ||
| status(): MeteorIDDPStream['currentStatus']; | ||
| statusChanged(): void; | ||
| reconnect(options?: unknown): void; | ||
| disconnect(options?: { _permanent?: boolean; _error?: unknown }): void; | ||
| _lostConnection(error?: unknown): void; | ||
| }; | ||
|
|
||
| type MeteorConnectionInternals = { | ||
| _stream: MeteorIDDPStream; | ||
| _streamHandlers: { | ||
| onMessage(raw: string): void; | ||
| onReset(): void; | ||
| }; | ||
| }; | ||
|
|
||
| if (isSdkTransportEnabled()) { | ||
| installStubMeteorStream(); | ||
| } | ||
|
|
||
| function installStubMeteorStream(): void { | ||
| const conn = Meteor.connection as unknown as MeteorConnectionInternals; | ||
| const conn = Meteor.connection; | ||
|
|
||
| const realStream = conn._stream; | ||
| const realStream = conn._stream!; | ||
|
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. 🧩 Analysis chain🏁 Script executed: cat -n apps/meteor/client/meteor/overrides/stubMeteorStream.tsRepository: RocketChat/Rocket.Chat Length of output: 12072 Avoid hard non-null assertion on The stream may not be initialized when this module loads. If Suggested fix- const realStream = conn._stream!;
+ const realStream = conn._stream;
+ if (!realStream) {
+ return;
+ }🤖 Prompt for AI Agents |
||
|
|
||
| // Carry Meteor's already-registered handlers (registered in the Connection | ||
| // constructor BEFORE we got a chance to swap `_stream`) over to the stub — | ||
|
|
@@ -76,11 +48,11 @@ function installStubMeteorStream(): void { | |
| // already closed / never opened | ||
| } | ||
|
|
||
| const eventCallbacks: Record<string, Array<(...args: unknown[]) => void>> = Object.create(null); | ||
| const eventCallbacks: Record<string, Array<(...args: any[]) => void>> = Object.create(null); | ||
| for (const [name, callbacks] of Object.entries(inheritedCallbacks)) { | ||
| eventCallbacks[name] = (callbacks as Array<(...args: unknown[]) => void>).slice(); | ||
| eventCallbacks[name] = callbacks.slice(); | ||
| } | ||
| const fire = (name: string, ...args: unknown[]): void => { | ||
| const fire = (name: string, ...args: any[]): void => { | ||
| const list = eventCallbacks[name]; | ||
| if (!list) return; | ||
| list.slice().forEach((cb) => cb(...args)); | ||
|
|
@@ -89,14 +61,14 @@ function installStubMeteorStream(): void { | |
| const TrackerDependency = (Tracker as unknown as { Dependency?: new () => { changed(): void } }).Dependency; | ||
| const statusListeners = TrackerDependency ? new TrackerDependency() : undefined; | ||
|
|
||
| const stub: MeteorIDDPStream = { | ||
| conn._stream = { | ||
| currentStatus: { | ||
| status: 'connected', | ||
| connected: true, | ||
| retryCount: 0, | ||
| }, | ||
|
|
||
| eventCallbacks, | ||
| eventCallbacks: eventCallbacks as NonNullable<typeof conn._stream>['eventCallbacks'], | ||
| statusListeners, | ||
|
|
||
| on(name, callback) { | ||
|
|
@@ -141,8 +113,6 @@ function installStubMeteorStream(): void { | |
| }, | ||
| }; | ||
|
|
||
| conn._stream = stub; | ||
|
|
||
| const bridgePongFor = (id?: string): void => { | ||
| conn._streamHandlers.onMessage( | ||
| DDPCommon.stringifyDDP({ msg: 'pong', ...(id != null && { id }) } as unknown as Parameters<typeof DDPCommon.stringifyDDP>[0]), | ||
|
|
||
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.
🧩 Analysis chain
🏁 Script executed:
Repository: RocketChat/Rocket.Chat
Length of output: 16209
🌐 Web query:
In Meteor's livedata package, in what order does Connection register its_stream.on('reset', ...)handler relative to when client code can attach listeners via_stream.on('reset', ...)? Does Meteor.status().connected become true synchronously before or after externalresetlisteners run?💡 Result:
No definitive source explicitly states the exact registration order of the Connection's internal
_stream.on('reset', ...)handler relative to client-attached listeners viaconnection._stream.on('reset', ...), nor the synchronous timing ofMeteor.status().connectedbecoming true relative to external reset listeners firing. However, based on standard Node.js EventEmitter behavior and Meteor's DDP client architecture from available sources, the following can be inferred with moderate confidence. The Connection's internal handler is registered during theConnectionconstructor inpackages/ddp-client/common/livedata_connection.js, shortly after creating the_stream(ClientStream instance) [1][2][3]. Client code attaching listeners via_stream.on('reset', ...)can occur at any time after the connection object is available, typically after Meteor.startup or via Tracker.autorun observing connection status [4][5]. Since Node.js EventEmitter adds listeners to an internal array in the orderon()is called, if client code attaches its listener after the constructor runs (which is the standard case), the internal handler is first in the list and executes before external (client) listeners when a 'reset' event fires [Node.js EventEmitter docs implied by general knowledge, confirmed by Meteor's use of streams]. ForMeteor.status().connected: This becomes true synchronously when the DDP 'connected' message is fully processed in_processOneDataMessage, which occurs after the stream's 'reset' event (triggered on new transport connection or protocol reset) [1][5][3]. The reset handlers run first upon stream reset, then DDP negotiation completes, 'connected' message arrives, status updates to true. Thus, status.connected becomes true after external reset listeners run. This sequence ensures internal reset logic (e.g., clearing stores, method invokers [2][6]) completes before client code reacts and before status reflects full connection readiness [4][5]. No changes noted in recent Meteor 3.x discussions [1][6].Citations:
Meteor.connection._processOneDataMessageget eliminated? meteor/meteor#13655retry: falsemeteor/meteor#14192🏁 Script executed:
Repository: RocketChat/Rocket.Chat
Length of output: 4941
🏁 Script executed:
Repository: RocketChat/Rocket.Chat
Length of output: 4256
Add defensive queueMicrotask to defer status check until after Meteor's internal reset handler completes.
The stream emits
'reset'when a new DDP session begins, triggering all registered'reset'listeners in order. Meteor's internalConnectionhandler updates the reactivestatusvariable, but this update is not synchronous relative to listener invocation—external listeners fire while Meteor's internal reset logic is still pending. The handler at line 47 checkssafeMeteorStatus()?.connected(line 66) synchronously and may see a stale value, causing the'connected'event emission (line 69) to be missed until the next status change. The'connection'event at line 71 still fires as a fallback.Wrap the callback invocation with
queueMicrotaskto defer status evaluation until after the listener call stack unwinds:Suggested fix
This ensures
safeMeteorStatus()?.connectedreflects Meteor's updated state when checked.🤖 Prompt for AI Agents