refactor: one silent fallback logger in lib/log - #261
Closed
diegolmello wants to merge 2 commits into
Closed
Conversation
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
lib/log.tsheld two separate implementations of a logger that writes nothing: theInternalLogclass, instantiated once to be the initial value, and an object literal thatsilence()rebuilt on every call. They had already drifted — one returnedundefinedand routed the legacywarnthroughwarning, the other returnednulland gavewarnits own empty body. Both were invisible, because neither wrote anything.There is now one value.
silentLoggeris the initial value ofloggerand is whatsilence()installs.InternalLogis deleted. The module's public surface is unchanged —logger,replaceLogandsilence, with the signatures they already had — so nothing that imports it has to change.Judgement calls, recorded in ADR-0005:
loggerto the Client, or callreplaceLog.silentLoggeris not exported. The suite already has its own recording logger for Sockets and Drivers, so nothing outside the module needs the value, and a second exported logger by that name would be easy to import in place of that one by mistake.replaceLogstill offers no way back to the previous logger.silence()returns to the fallback, not to whatever was installed before. No caller needs more, so no new surface was added for it.One behaviour change worth naming:
silence()used to install a fresh object each call and now installs the shared fallback, so mutating a method onloggeraftersilence()would affect every later fallback rather than one. The initial value was already a single shared instance, so this narrows an existing hazard rather than adding one.Fallback loggeris added to CONTEXT.md — the concept was load-bearing across the Socket and Driver constructors but had no name.lib/emitter.tswas reviewed in the same pass and deliberately left alone. Its two overrides are already covered by ADR-0002 and pinned exhaustively by its spec; the only change worth making there — collapsingoff's two lookups into the one predicateremoveAllListenersalready uses — is behaviour-identical and would need the ADR reworded for no gain.No pinning test changed. The Driver suite is untouched, and this module had no spec before.
Steps to reproduce
npm run lintnpm run typechecknpm testTests
lib/__tests__/log.spec.ts— new spec for the module, which had none:ILoggercontract names, including the legacywarnconsolemethodreplaceLogis seen by everything that readsloggerafterwardssilenceundoes a replacement, back to the same fallback the SDK started with