feat: build replacement ddp server#1789
Open
Julusian wants to merge 5 commits into
Open
Conversation
Julusian
added
the
Contribution from SuperFly.tv
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (47)
💤 Files with no reviewable changes (4)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (38)
WalkthroughThis PR removes class-based ChangesRegistry-based method/publication wiring and standalone DDP server
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant DDPClient
participant DdpSession
participant MethodRegistry
participant PublicationRegistry
participant DdpPublicationContext
DDPClient->>DdpSession: connect (DDP version)
DdpSession-->>DDPClient: connected
DDPClient->>DdpSession: method message
DdpSession->>MethodRegistry: get(methodName)
MethodRegistry-->>DdpSession: handler
DdpSession-->>DDPClient: result + updated
DDPClient->>DdpSession: sub message
DdpSession->>PublicationRegistry: get(publicationName)
PublicationRegistry-->>DdpSession: publication callback
DdpSession->>DdpPublicationContext: create context
DdpPublicationContext->>DdpSession: added/changed/removed
DdpSession-->>DDPClient: added/changed/removed, ready
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
with some renaming. this makes them reusable for a ddp server implementation
Julusian
force-pushed
the
feat/replace-ddp-server
branch
4 times, most recently
from
8d5b516 to
e6916ef
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
meteor/server/ddp-server/__tests__/clientServerIntegration.test.ts (1)
26-33: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the fixed 30ms wire delay with condition-based waiting.
flushWire()makes these integration assertions scheduler-dependent, so this suite can get flaky on slower CI workers. Since the file already haswaitFor, prefer waiting for the actual state/callback change instead of sleeping a fixed interval.Suggested direction
- const flushWire = () => new Promise((resolve) => setTimeout(resolve, 30)) const waitFor = async (cond: () => boolean, timeoutMs = 3000): Promise<void> => { const start = Date.now() while (!cond()) { if (Date.now() - start > timeoutMs) throw new Error('waitFor timed out') await new Promise((r) => setTimeout(r, 20)) } }Then use
waitFor(...)at the call sites, e.g. wait forupdatedto be called or forclient.collections['TestColl']?.['d1']to change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meteor/server/ddp-server/__tests__/clientServerIntegration.test.ts` around lines 26 - 33, Replace the fixed-delay helper `flushWire()` in `clientServerIntegration.test.ts` with condition-based waiting using the existing `waitFor()` utility, so the tests wait for the actual callback/state transition instead of sleeping 30ms. Update the call sites in the integration tests to await the relevant condition (for example, `updated` being called or `client.collections['TestColl']?.['d1']` changing), and remove any reliance on scheduler timing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@meteor/__mocks__/helpers/methods.ts`:
- Around line 12-15: Reset the shared Meteor method mock state before applying
the registry in registerAllMethodsForTest. The current helper only calls
registerAllApiMethods and registry.applyToMeteor, so MeteorMock.mockMethods can
retain stale entries between test runs; clear or replace that mock table as part
of this helper so each suite starts from a clean MethodRegistry-backed state.
In `@meteor/__mocks__/meteor.ts`:
- Around line 233-235: The Meteor mock’s onConnection is currently a no-op,
which drops handlers registered by Connections.ts at import time. Update
meteor/__mocks__/meteor.ts so onConnection stores the provided callbacks instead
of ignoring them, and add a small test helper to invoke the captured callbacks
when needed. Keep the fix localized around onConnection and any new helper so
tests can exercise connection-open/close tracking behavior.
In `@meteor/server/api/__tests__/userActions/general.test.ts`:
- Around line 8-10: Keep the deviceTriggers observer mocked in this test suite
before calling registerAllMethodsForTest(), because the helper now registers all
APIs and will otherwise start extra background observers that interfere with the
fake-timer/log assertions. Update the general.test.ts setup to follow the same
pattern used by sibling suites: mock ../../deviceTriggers/observer first, then
invoke registerAllMethodsForTest() so only the intended methods are active
during these tests.
In `@meteor/server/api/__tests__/userActions/system.test.ts`:
- Around line 25-27: Mock deviceTriggers/observer before calling
registerAllMethodsForTest() in system.test.ts, since that helper now pulls in
the broader method graph and can trigger unwanted side effects. Add the same
jest.mock('../../deviceTriggers/observer') shielding used in other affected
suites so the test setup stays isolated and background state does not leak
between cases.
In `@meteor/server/collections/implementations/asyncCollection.ts`:
- Around line 28-29: The publication cursor contract in AsyncCollection still
allows unnamed collections via collectionName: string | null, which lets
findWithCursor() return a cursor that will later fail in
driveSubscriptionFromCursor(). Tighten the cursor type and assignment in
asyncCollection.ts so collectionName is always a string, or explicitly throw in
findWithCursor() / the cursor-building path when this.name is missing before
returning the publication cursor. Use the existing collectionName and
findWithCursor symbols to locate the affected code, and ensure the same fix
covers the related assignment around the publication cursor return.
In `@meteor/server/ddp-server/ConnectionRegistry.ts`:
- Around line 18-25: Guard the lifecycle hooks in ConnectionRegistry so they
only run when the Set actually changes. In add() and remove(), use the return
value of this.sessions.add/delete to make the operations idempotent, and call
hooks.onOpen or hooks.onClose only when the session was newly added or actually
removed. This keeps DdpSession.handleConnect and DdpSession.close from
double-counting or firing onClose for sessions that never completed connect.
In `@meteor/server/ddp-server/methodDispatch.ts`:
- Around line 35-40: Delay the `updated` message in `methodDispatch` until the
method’s publication side effects have been flushed to the session, instead of
sending it immediately after `handler.apply(...)`. Update the flow around the
method result/`updated` send path so `ddpClient` will only see `updated` after
the matching `added`/`changed`/`removed` messages are deliverable, preserving
Meteor wire ordering. Use the existing method dispatch logic and the
`updatedCallbacks`/session flush point to place the barrier correctly.
In `@meteor/server/lib/clientAddress.ts`:
- Around line 28-31: The `parseClientAddress` helper is only returning the last
character of the RFC 7239 `for=` value because the repeated capture in the regex
overwrites `match[2]`. Update the regex in `parseClientAddress` so the entire
address is captured once, then return that full captured value instead of a
single character. Make sure the fix still handles bracketed IPv6 forms, since
`DdpConnection` and `koa` both rely on this helper for `clientAddress`.
In `@meteor/server/methodRegistry.ts`:
- Around line 67-95: `MethodRegistry.registerApi()` is mutating `this.methods`
incrementally, so a failure in one class member can leave earlier methods
registered and violate the atomic registration guarantee. Refactor
`registerApi()` to first validate/build the full set of entries for `orgClass`
(using `getAllClassMethods`, `methodEnum`, and any `wrapper`/`secret` handling)
without calling `this.add`, then only commit them to `this.methods` after all
methods are confirmed valid. Keep the existing error logging in the catch block,
and make sure any duplicate or missing wire-name error aborts the entire API
class registration before any partial state is stored.
In `@meteor/server/publicationRegistry.ts`:
- Around line 170-174: Exclude customPublish() entries from the legacy
REST/introspection signature path by updating the publication signature lookup
in PublicationRegistry so it applies the same isCustom filter as
getCursorPublication(). Locate the method that returns publication signatures
(the one around the exposed signatures block) and ensure it skips any
publication marked custom before returning or collecting its signature, so only
legacy cursor-backed publications are advertised.
In `@meteor/server/publications/rundown.ts`:
- Around line 59-63: The `partInstancesSimple` cursor is using
`pieceInstanceFields`, which only excludes `piece.*` paths and leaves
`part.privateData` included in `DBPartInstance` results. Update the projection
used by `partInstancesSimple` to a `DBPartInstance`-specific field specifier
that explicitly excludes the nested `part.privateData` data (and any other
`part.*` fields that should be omitted), instead of spreading
`pieceInstanceFields`; keep the fix localized around `partInstancesSimple` and
the projection constants near `pieceInstanceFields`.
In `@meteor/server/publications/timeline.ts`:
- Around line 41-45: The registry.publish callbacks in timeline.ts are missing
the trailing _token parameter, which changes the public signature derived by
PublicationRegistry. Update each affected callback (including the
timelineDatastore publication and the other registry.publish handlers in this
file) to accept the unused _token argument after context so the exposed
signature stays compatible for legacy REST/introspection consumers.
In `@packages/webui/src/meteor/socket-stream-client/urls.js`:
- Around line 53-57: The toWebsocketUrl/translateUrl path is no longer
preserving the DDP endpoint for root or base URLs, so explicit DDP.connect
callers can end up on the site root instead of /websocket. Update toWebsocketUrl
in urls.js so it still appends /websocket for base inputs like '/',
'http://host', and ddp+sockjs://host, or otherwise ensure every caller passes a
full endpoint path; verify the behavior around translateUrl and the exported
toWebsocketUrl function.
---
Nitpick comments:
In `@meteor/server/ddp-server/__tests__/clientServerIntegration.test.ts`:
- Around line 26-33: Replace the fixed-delay helper `flushWire()` in
`clientServerIntegration.test.ts` with condition-based waiting using the
existing `waitFor()` utility, so the tests wait for the actual callback/state
transition instead of sleeping 30ms. Update the call sites in the integration
tests to await the relevant condition (for example, `updated` being called or
`client.collections['TestColl']?.['d1']` changing), and remove any reliance on
scheduler timing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0009862-e69a-46f4-8cfe-c553f85ea182
⛔ Files ignored due to path filters (3)
meteor/server/__tests__/__snapshots__/methodRegistry.test.ts.snapis excluded by!**/*.snapmeteor/server/__tests__/__snapshots__/publicationRegistry.test.ts.snapis excluded by!**/*.snapmeteor/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (120)
meteor/.meteor/packagesmeteor/__mocks__/_setupMocks.tsmeteor/__mocks__/helpers/methods.tsmeteor/__mocks__/helpers/publications.tsmeteor/__mocks__/meteor-ejson.tsmeteor/__mocks__/meteor.tsmeteor/__mocks__/mongo.tsmeteor/package.jsonmeteor/scripts/babel-jest.jsmeteor/server/Connections.tsmeteor/server/__tests__/Connections.test.tsmeteor/server/__tests__/cronjobs.test.tsmeteor/server/__tests__/methodRegistry.test.tsmeteor/server/__tests__/publicationRegistry.test.tsmeteor/server/api/ExternalMessageQueue.tsmeteor/server/api/__tests__/client.test.tsmeteor/server/api/__tests__/externalMessageQueue.test.tsmeteor/server/api/__tests__/peripheralDevice.test.tsmeteor/server/api/__tests__/rundownLayouts.test.tsmeteor/server/api/__tests__/userActions/general.test.tsmeteor/server/api/__tests__/userActions/system.test.tsmeteor/server/api/blueprints/__tests__/api.test.tsmeteor/server/api/blueprints/api.tsmeteor/server/api/client.tsmeteor/server/api/ingest/debug.tsmeteor/server/api/mongo.tsmeteor/server/api/peripheralDevice.tsmeteor/server/api/playout/api.tsmeteor/server/api/playout/debug.tsmeteor/server/api/rest/api.tsmeteor/server/api/rest/koa.tsmeteor/server/api/rest/v0/__tests__/rest.test.tsmeteor/server/api/rest/v0/index.tsmeteor/server/api/rundown.tsmeteor/server/api/rundownLayouts.tsmeteor/server/api/showStyles.tsmeteor/server/api/snapshot.tsmeteor/server/api/studio/api.tsmeteor/server/api/system.tsmeteor/server/api/triggeredActions.tsmeteor/server/api/user.tsmeteor/server/api/userActions.tsmeteor/server/collections/implementations/asyncCollection.tsmeteor/server/collections/implementations/mock.tsmeteor/server/ddp-server/ConnectionRegistry.tsmeteor/server/ddp-server/DdpConnection.tsmeteor/server/ddp-server/DdpPublicationContext.tsmeteor/server/ddp-server/DdpSession.tsmeteor/server/ddp-server/Heartbeat.tsmeteor/server/ddp-server/SessionCollectionView.tsmeteor/server/ddp-server/SessionDocumentView.tsmeteor/server/ddp-server/__tests__/DdpConnection.test.tsmeteor/server/ddp-server/__tests__/DdpPublicationContext.test.tsmeteor/server/ddp-server/__tests__/DdpSession.test.tsmeteor/server/ddp-server/__tests__/Heartbeat.test.tsmeteor/server/ddp-server/__tests__/SessionCollectionView.test.tsmeteor/server/ddp-server/__tests__/clientServerIntegration.test.tsmeteor/server/ddp-server/__tests__/methodDispatch.test.tsmeteor/server/ddp-server/__tests__/subscriptionDispatch.test.tsmeteor/server/ddp-server/__tests__/wireCodec.test.tsmeteor/server/ddp-server/config.tsmeteor/server/ddp-server/index.tsmeteor/server/ddp-server/methodDispatch.tsmeteor/server/ddp-server/subscriptionDispatch.tsmeteor/server/ddp-server/wireCodec.tsmeteor/server/lib/clientAddress.tsmeteor/server/lib/customPublication/index.tsmeteor/server/lib/customPublication/publish.tsmeteor/server/main.tsmeteor/server/methodRegistrations.tsmeteor/server/methodRegistry.tsmeteor/server/methods.tsmeteor/server/migration/__tests__/migrations.test.tsmeteor/server/migration/api.tsmeteor/server/publicationRegistrations.tsmeteor/server/publicationRegistry.tsmeteor/server/publications/_publications.tsmeteor/server/publications/blueprintUpgradeStatus/publication.tsmeteor/server/publications/buckets.tsmeteor/server/publications/deviceTriggersPreview.tsmeteor/server/publications/externalEventSubscriptions.tsmeteor/server/publications/ingestStatus/publication.tsmeteor/server/publications/lib/lib.tsmeteor/server/publications/mountedTriggers.tsmeteor/server/publications/organization.tsmeteor/server/publications/packageManager/expectedPackages/publication.tsmeteor/server/publications/packageManager/packageContainers.tsmeteor/server/publications/packageManager/playoutContext.tsmeteor/server/publications/partInstancesUI/publication.tsmeteor/server/publications/partsUI/publication.tsmeteor/server/publications/peripheralDevice.tsmeteor/server/publications/peripheralDeviceForDevice.tsmeteor/server/publications/pieceContentStatusUI/bucket/publication.tsmeteor/server/publications/pieceContentStatusUI/rundown/publication.tsmeteor/server/publications/rundown.tsmeteor/server/publications/rundownPlaylist.tsmeteor/server/publications/segmentPartNotesUI/publication.tsmeteor/server/publications/showStyle.tsmeteor/server/publications/showStyleUI.tsmeteor/server/publications/studio.tsmeteor/server/publications/studioUI.tsmeteor/server/publications/system.tsmeteor/server/publications/timeline.tsmeteor/server/publications/translationsBundles.tsmeteor/server/publications/triggeredActionsUI.tsmeteor/server/security/auth.tsmeteor/server/security/check.tsmeteor/server/security/securityVerify.tsmeteor/server/systemStatus/__tests__/api.test.tsmeteor/server/systemStatus/__tests__/systemStatus.test.tsmeteor/server/systemStatus/api.tspackages/meteor-lib/src/userPermissions.tspackages/server-core-integration/src/__mocks__/ws.tspackages/server-core-integration/src/lib/ddpClient.tspackages/shared-lib/src/ddp/messageTypes.tspackages/webui/src/client/collections/lib.tspackages/webui/src/meteor/ddp/client_convenience.jspackages/webui/src/meteor/meteor.d.tspackages/webui/src/meteor/socket-stream-client/urls.jspackages/webui/vite.config.mts
💤 Files with no reviewable changes (6)
- meteor/.meteor/packages
- packages/meteor-lib/src/userPermissions.ts
- meteor/mocks/meteor-ejson.ts
- packages/webui/src/meteor/meteor.d.ts
- packages/webui/src/client/collections/lib.ts
- meteor/server/publications/_publications.ts
Julusian
force-pushed
the
feat/replace-ddp-server
branch
from
e6916ef to
6fbffcf
Compare
for now intended to run at a secondary path off the meteor ddp server
Julusian
force-pushed
the
feat/replace-ddp-server
branch
from
6fbffcf to
35ff5a3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
About the Contributor
Type of Contribution
This pull request is posted on behalf of Superfly
This is part of a series of PRs aiming to replace meteor.
This builds upon #1785 & #1783 (a majority of this diff is from these)
Current Behavior
We are relying on meteor for the ddp driver. To be able to leave meteor we need our own standalone implementation
New Behavior
This builds a from scratch ddp server.
Following on from #1785 means that we already have a single registry of all the publications and methods to register on the server.
Some ddp message typings from server-core-integration have been moved into shared-lib so that the server side can use the same types.
I haven't yet figured out how to disable the meteor default ddp server, so this is currently opt-in under the path
/ddp-next/websocketover the existing meteor http server. If the env varSTANDALONE_DDP_SERVER_ENABLED=1is set, this will be passed through to the ui for use instead of the default server.This intentionally has touched the ui as little as possible, the only changes are to drop some already dead code and to support using this alternate path.
As a bonus of this, we are no longer constrained to the
dntheader for the auth implementation, so this is now configurable with theSOFIE_PERMISSIONS_HEADERenv var, but defaults todntfor backwards compatibility.There are a couple of lines of overlap here with #1786, specifically around publishing a cursor, so I expect a minor merge conflict between them. It should generally be safe though, as the cursor publishing contract is the same in both branches.
Testing
Affected areas
Time Frame
Other Information
Status