Fix: replace winston with a console logger in core to unbreak the web build - #617
Closed
AmaadMartin wants to merge 3 commits into
Closed
Fix: replace winston with a console logger in core to unbreak the web build#617AmaadMartin wants to merge 3 commits into
AmaadMartin wants to merge 3 commits into
Conversation
added 3 commits
August 4, 2026 14:34
… build core/src/utils/logger.ts imported winston, and the web build is a transpile-only esbuild pass, so the specifier was copied verbatim into dist/web/utils/logger.js. Winston transitively pulls in os, fs, util, zlib and http, so any bundler targeting the browser failed to resolve the published web artifact. SimpleLogger only used winston for a printf template; the level filter was already done by hand on an instance field. Rewriting it on console keeps the line layout, the default INFO level and the public export surface unchanged, and lets the winston dependency be dropped from core. Collapsing the four level methods into log() also fixes a latent crash: log() passed the numeric level as a winston level name, which made logform's colorizer throw. dev/ keeps its own winston logger, so node_modules/winston and the dev lockfile entry are untouched.
Adds a new SimpleLogger describe block alongside the existing setLogger block, which is left untouched. Covers level routing to the matching console method, the level filter in both directions, the INFO default, argument joining, formatLogLine, and log() no longer throwing.
…h a lookup Review follow-up. formatLogLine was an exported module-level helper with a single production caller; the format it existed to make testable is already pinned end to end by the full-line regex assertions on the console spies, so the seam bought no coverage. Inlined it and the single-use LOG_LABEL constant into log(). The exhaustive four-case switch becomes a CONSOLE_METHOD lookup. The console method is still resolved at call time, so vi.spyOn(console, ...) still intercepts, and the lookup is strictly stricter than the switch it replaces: the switch had no default branch and so accepted a newly added LogLevel silently, while indexing the lookup with an uncovered member is a compile error. The test that exercised formatLogLine directly is re-pointed at the public API rather than dropped, keeping the case count at 15.
Collaborator
|
No, we need to keep winston! Please close this pr |
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.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
winstonhalf only. This is deliberatelya "part of" link and not a closing keyword, because the other half of that
issue (the
node:async_hooksimport incore/src/utils/client_labels.ts) isnot fixed here and is still outstanding. Web build leaks Node-only deps: the async_hooks shim is gated behind --bundle (never applied), and winston has no shim #611 must stay open. See
"What this does not fix" below.
2. Or, if no issue exists, describe the change:
Problem:
@google/adkadvertises a browser build ("browser": "./dist/web/index_web.js"),but that artifact ships a literal
import * as winston from 'winston'.core/build.jsis a transpile-only esbuild pass (bundle: false,packages: 'external'), so module specifiers are copied through verbatim intocore/dist/web/utils/logger.js. Winston transitively drags inos,fs,util,zlibandhttp, so any bundler targeting the browser fails toresolve the published web artifact.
A build-layer alias is not available.
core/build.jsgates the existingnode:async_hooksalias behindplatform === 'browser' && bundle, and thatgate is load-bearing: esbuild rejects
aliasoutright withoutbundle(
✘ [ERROR] Cannot use "alias" without "bundle"). That approach was alreadyattempted and corrected on the issue thread, so it is not re-attempted here.
Solution:
Drop the dependency at the source rather than patch the build for one platform
(Option A on #611).
SimpleLoggeronly used winston for two things: a printf template, and a"level filter" that was already a pass-through — the real filter is the
instance field
private logLevel, and every method early-returned beforetouching winston. Rewriting it on
consoleletswinstonleavecore'sdependencies entirely.
core/src/utils/logger.tsnow imports nothing at all — nowinston, nonode:*builtin, noprocessreference. The four level methods collapse intoa single
log(), which builds the line inline and dispatches through a smallCONSOLE_METHODlookup. Theconsolemethod is resolved at call time (nevercaptured into a module-level constant), so
vi.spyOn(console, ...)stillintercepts it. That lookup replaced an exhaustive four-case
switchand isstrictly stricter than it: the
switchhad nodefaultbranch and so wouldaccept a newly added
LogLevelsilently, whereas indexing the lookup with anuncovered member is a compile error — verified by temporarily adding a fifth
member:
Nothing is added to
common.tsorindex.ts, so the public export surface isunchanged.
Behaviour differences (all four intentional):
winstonis no longer a dependency of@google/adk. This is the pointof the change. A consumer that relied on winston being installed
transitively via
@google/adkmust now declare it directly.@google/adk-devtoolsdeclares it itself, so it is unaffected.\x1b[3Xm...\x1b[39m. The replacement emits plain text. ANSI escapes renderas literal garbage in a browser devtools console — the environment this
change exists to support — and the
devCLI keeps its own colourisedwinston logger for terminal output. The line's text layout is unchanged:
LEVEL: [ADK] <ISO-8601> <message>, with the samenew Date().toISOString()timestamp winston's defaultformat.timestamp()produced.
warnanderrornow go to stderr. winston'sConsoletransport putall four levels on stdout (its
stderrLevelsset is empty for these customlevels).
console.warn/console.errorwrite to stderr in Node and lightup the corresponding devtools level in a browser.
logger.log(level, ...)no longer throws. It previously passed thenumeric level (
'0'..'3') as a winston level name, which is notregistered; winston logged
[winston] Unknown logger level: 1and logform'scolorizer then threw
TypeError: colors[Colorizer.allColors[lookup]] is not a function. Noexisting test covered that path. Collapsing the four methods into one
log()fixes it incidentally, and there is now a regression test for it.Not changed: message text, level-filter semantics (
this.logLevel > level),the
LogLevel.INFOdefault, theSimpleLogger/NoOpLoggerclass names(existing tests assert
constructor.name), the public export surface, and all41
core/srccall sites — no call-site edits.Scope kept tight, deliberately. No env-var/config knob for the log level,
no
typeof console !== 'undefined'guard, no ANSI support, no change tocore/build.js,index_web.ts,common.ts,index.ts, or anything underdev/. No suppressions of any kind were added — noany,@ts-expect-error,eslint-disableor coverage pragma appears in this diff(
eslint.config.jshas nono-consolerule, soconsole.*inside the loggerlints clean without one).
What this does not fix —
node:async_hooks.Removing winston does not make the web build bundleable, and this PR does
not claim to. The second Node-only leak described in #611 is untouched and
still present in the built artifact:
It is pulled in by
core/src/utils/client_labels.ts, which is reachable fromthe web entry point via
index_web.ts→export * from './common.js'→core/src/common.ts(export {getClientLabels, runWithClientLabel} from './utils/client_labels.js').core/src/utils/async_hooks_shim.tsexists andis correct, but is only wired through
buildOptions.alias, which esbuildrefuses without
bundle— so it is dead in every published artifact. That isa separate change; #611 should stay open for it.
Lockfile. The only
package-lock.jsonchange is the single"winston": "^3.19.0",line inside the"core"workspace'sdependenciesblock (
1 file changed, 0 insertions(+), 1 deletion(-)). The"dev"workspaceblock and the
node_modules/winstonentry are intentionally untouched, becausedevstill depends on winston.npm ciwas used (nevernpm install) andpasses, which is itself the proof the edit is correct and complete.
Testing Plan
Unit Tests:
Seven tests were added as a new top-level
describe('SimpleLogger', ...)block in the existing
core/test/utils/logger_test.ts. Nothing in the existingdescribe('setLogger', ...)block was edited, weakened, skipped or deleted —it passes unchanged.
Coverage of
core/src/utils/logger.ts: 100% branch, 89.87% statements.Every uncovered statement is inside the pre-existing deprecated
loggerconstwrapper at the bottom of the file, which this change does not touch. All new
code — every branch of
SimpleLogger— is 100% line- and branch-covered.Proving each test can fail. Two techniques were needed, because the winston
baseline never touches
consoleat all and would pass negative assertionsvacuously.
Against the unfixed source (restore
logger.tsfrommain, re-run, restore)— all 7 new tests fail, the 8 pre-existing ones still pass:
Test 6 reproduces the real crash described above. Test 2 fails on its
positive half — a "not called" assertion alone would have passed vacuously
here, which is why both halves are asserted in one test.
By mutating the new source (each mutation reverted afterwards):
if (this.logLevel > level) { return; }fromSimpleLogger.logsuppresses a message below the configured level,defaults to INFO— 2 failed | 13 passedprivate logLevel: LogLevel = LogLevel.DEBUGdefaults to INFO— 1 failed | 14 passedmessages.join(',')instead ofjoin(' ')joins arguments with a single space— 1 failed | 14 passed[LogLevel.WARN]at'info'in theCONSOLE_METHODlookupsuppresses a message below the configured level,routes each level to its matching console method,formats the full line for a warning— 3 failed | 12 passed[ADK]label from the line templateemits a message at the configured level,routes each level to its matching console method,joins arguments with a single space,formats the full line for a warning— 4 failed | 11 passedCollateral check. The default level is
INFO, so core's logger output nowflows through
console.info/warn/errorin-process. The fourdevtestfiles that spy on those methods were run to confirm the extra calls break
nothing:
Manual End-to-End (E2E) Tests:
The build artifact is the end-to-end test for this fix — no mocks, the real
published-shape output:
Before, all nine hits in one file:
After: no output.
The issue's bundler repro was also run against the built web entry point.
Locally
winstonitself still resolves (thedevworkspace keeps it innode_modules, so esbuild walks into it rather than reportingCould not resolve "winston"the way a published-package consumer would); themeasurable effect is that winston's transitive Node-builtin imports disappear
from the unresolved set — 345 → 325 unresolved import sites:
utilosfshttpspathhttpzlibNo module disappears from the list entirely, because every one of those
builtins is also reached through other dependencies and through the
node:async_hookschain that this PR reports rather than fixes. This isexactly why this PR is
Part of #611and not a fix for it.Other gates, run locally on the exact commit pushed:
npm ci(exit 0),npm run build(exit 0),npx eslint core/src/utils/logger.ts core/test/utils/logger_test.ts(clean),npm run format:check(clean, repo-wide),npm run docs:check(exit 0),bash scripts/check_license.sh(clean),npx secretlint(clean).Checklist
Additional context
The companion
node:async_hookshalf of #611 is intentionally left for aseparate change. #611 should therefore stay open after this merges.