refactor: give each duplicated fact a single owner - #432
Merged
Merged
Conversation
# Conflicts: # docs/adr/0006-a-sub-abandoned-by-a-forced-reconnect-keeps-its-entry.md # lib/drivers/__tests__/ddp.subscriptions.spec.ts # lib/drivers/__tests__/ddpSubscriptions.spec.ts # lib/drivers/__tests__/driver.login.spec.ts # lib/drivers/__tests__/driver.spec.ts # lib/drivers/__tests__/driver.streams.spec.ts # lib/drivers/__tests__/socket.connection.spec.ts # lib/drivers/__tests__/socket.sendDuringRecovery.spec.ts # test/fakeTransport.ts
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
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.
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/deletewere four byte-identical fetch bodies differing only in the verb. They now call one privaterequest.Api.request's verb switch became a lookup map.Socket(close, probe, open) shared one shape: a settled flag, a timer, a cleanup. They now share onelatchedByDeadline, and each caller contributes only its own listener wiring.SocketandConnectionwith the same log message. Both now go throughcloseTransportIntentionally, exported fromconnection.ts.DDPRequests.send: a regex, an&&chain with a different membership, then the regex again. The two lists disagreed onpong. Now two named constants, in the terms CONTEXT.md uses.probehand-rolled a raw write plus its ownonce('pong')wait, duplicatingDDPRequests.send, whichping()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.InternalLogandsilence()built two equivalent sets of no-ops; one shared no-op logger replaces both.JSON.stringifyon request payloads and errors before handing them to a logger that is a no-op by default.DDPRequests.send's defaultdeadlineMswas unreachable:Socket.sendalways passes a concrete value. Removed, along with the duplicated field.Tests:
createSocketfixture replaces nine local factories and fourteen call sites, plus the constants four spec files had each re-declared.ddpRequests.spec.tsandddpSubscriptions.spec.tsexercised 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 realSocketand the fake transport. One behavior that was genuinely uncovered got an assertion inddp.subscriptions.spec.ts.socket.connection.spec.tscovered four concerns in 1246 lines; it is nowsocket.open,socket.reopen,socket.closeandsocket.events, each announcing itself in its own describe.driver.spec.tsgave up itswaitForNotifyUserMediaSubshalf todriver.mediaSubs.spec.ts.One real fix, from reviewing the third commit:
msg.includes(...)throws aTypeErroron a frame with nomsg, where the regex it replaced returned false.Socket.sendis public and takesany, so that path was reachable from a consumer.Left alone deliberately, each because it changes behavior a consumer can see:
onMessagecorrelates aresultframe twice, once reshaped and once raw. Collapsing to one rule changes whatsendresolves with and breaks seven pinning assertions.driver.onMessageanddriver.onTypingregister socket listeners with no removal path, so repeated foreground and reconnect cycles compound callbacks per frame. Fixing it changes theIDriversignature pinned by the consumer contract. Worth its own issue.DriverandSocket.hostToWSstrip the host scheme with two disagreeing regexes, andconfig.hostis public and pinned.probenow consumes an id and so increments the publicsentcounter, which previously counted only id-carrying traffic.Steps to reproduce
No behavior change to reproduce. To verify:
npm run typecheckpasses on all three tsconfigs.npm testpasses, 438 tests across 33 suites.npx jest --coverage --collectCoverageFrom='lib/**/*.ts'reportslib/driversbranch 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 againstddp.send.spec.ts: a message with nomsgis written with an allocated id and its reply resolves matched by that idddp.subscriptions.spec.ts: a resubscribe whose stream is never recorded sends nothing before its deadlinesocket.open.spec.ts,socket.reopen.spec.ts,socket.close.spec.ts,socket.events.spec.ts: the four concerns split out ofsocket.connection.spec.tssocket.construction.spec.ts,socket.loginParams.spec.ts: the two topics split out ofsocket.spec.tsdriver.mediaSubs.spec.ts: thewaitForNotifyUserMediaSubssuite split out ofdriver.spec.ts