Skip to content

refactor: give each duplicated fact a single owner - #432

Merged
diegolmello merged 12 commits into
mobilefrom
diegolmello/simplify
Sep 1, 2026
Merged

diegolmello merged 12 commits into
mobilefrom
diegolmello/simplify

Conversation

@diegolmello

Copy link
Copy Markdown
Member

Proposed changes

Ten commits of cleanup over the driver, the REST layer and the test suite. No public behavior changes.

Source:

  • Client.get/post/put/delete were four byte-identical fetch bodies differing only in the verb. They now call one private request. Api.request's verb switch became a lookup map.
  • Three hand-rolled deadline waits in Socket (close, probe, open) shared one shape: a settled flag, a timer, a cleanup. They now share one latchedByDeadline, and each caller contributes only its own listener wiring.
  • Closing a transport defensively lived in Socket and Connection with the same log message. Both now go through closeTransportIntentionally, exported from connection.ts.
  • Which DDP messages carry an id, and what answers them, was encoded three inconsistent ways inside DDPRequests.send: a regex, an && chain with a different membership, then the regex again. The two lists disagreed on pong. Now two named constants, in the terms CONTEXT.md uses.
  • probe hand-rolled a raw write plus its own once('pong') wait, duplicating DDPRequests.send, which ping() already used. It now routes through the same path, so an in-flight probe is abandoned on close instead of idling to its own deadline. Its deadline stays an argument.
  • InternalLog and silence() built two equivalent sets of no-ops; one shared no-op logger replaces both.
  • The REST layer no longer runs JSON.stringify on request payloads and errors before handing them to a logger that is a no-op by default.
  • DDPRequests.send's default deadlineMs was unreachable: Socket.send always passes a concrete value. Removed, along with the duplicated field.

Tests:

  • One shared createSocket fixture replaces nine local factories and fourteen call sites, plus the constants four spec files had each re-declared.
  • ddpRequests.spec.ts and ddpSubscriptions.spec.ts exercised the same behaviors as the socket-level suite through a stubbed send. Deleted, after mapping every behavior to the surviving test that covers it through a real Socket and the fake transport. One behavior that was genuinely uncovered got an assertion in ddp.subscriptions.spec.ts.
  • socket.connection.spec.ts covered four concerns in 1246 lines; it is now socket.open, socket.reopen, socket.close and socket.events, each announcing itself in its own describe. driver.spec.ts gave up its waitForNotifyUserMediaSubs half to driver.mediaSubs.spec.ts.
  • Removed the narration comments, the tautological cases, and the names that overclaimed what they asserted.

One real fix, from reviewing the third commit: msg.includes(...) throws a TypeError on a frame with no msg, where the regex it replaced returned false. Socket.send is public and takes any, so that path was reachable from a consumer.

Left alone deliberately, each because it changes behavior a consumer can see:

  • onMessage correlates a result frame twice, once reshaped and once raw. Collapsing to one rule changes what send resolves with and breaks seven pinning assertions.
  • driver.onMessage and driver.onTyping register socket listeners with no removal path, so repeated foreground and reconnect cycles compound callbacks per frame. Fixing it changes the IDriver signature pinned by the consumer contract. Worth its own issue.
  • Driver and Socket.hostToWS strip the host scheme with two disagreeing regexes, and config.host is public and pinned.
  • probe now consumes an id and so increments the public sent counter, which previously counted only id-carrying traffic.

Steps to reproduce

No behavior change to reproduce. To verify:

  • npm run typecheck passes on all three tsconfigs.
  • npm test passes, 438 tests across 33 suites.
  • npx jest --coverage --collectCoverageFrom='lib/**/*.ts' reports lib/drivers branch coverage at 96.55 percent, above the 96.13 percent this branch started from.

Tests

  • test/createSocket.ts, a shared socket fixture with the reopen delay, timeout and ping interval the specs assert against
  • ddp.send.spec.ts: a message with no msg is written with an allocated id and its reply resolves matched by that id
  • ddp.subscriptions.spec.ts: a resubscribe whose stream is never recorded sends nothing before its deadline
  • socket.open.spec.ts, socket.reopen.spec.ts, socket.close.spec.ts, socket.events.spec.ts: the four concerns split out of socket.connection.spec.ts
  • socket.construction.spec.ts, socket.loginParams.spec.ts: the two topics split out of socket.spec.ts
  • driver.mediaSubs.spec.ts: the waitForNotifyUserMediaSubs suite split out of driver.spec.ts

@diegolmello
diegolmello merged commit a4adc3b into mobile Sep 1, 2026
5 checks passed
@diegolmello
diegolmello deleted the diegolmello/simplify branch September 1, 2026 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant