Skip to content

fix: add setBindings to the browser logger - #2471

Merged
mcollina merged 1 commit into
pinojs:mainfrom
ankit1324:fix/browser-set-bindings
Jul 18, 2026
Merged

mcollina merged 1 commit into
pinojs:mainfrom
ankit1324:fix/browser-set-bindings

Conversation

@ankit1324

Copy link
Copy Markdown
Contributor

Fixes #2021

Problem

The browser logger (browser.js) never implemented setBindings, even though the TypeScript types declare it on LoggerExtras — so any code path that resolves the browser field of package.json throws TypeError: logger.setBindings is not a function. (To answer the question in the issue thread: it's a JS bug — a missing method in the browser implementation, not a typings problem. The named/default import distinction in the report was a red herring; both exports point at the same function, what differs is whether the bundler picked pino.js or browser.js.)

Fix

Implement setBindings on the browser logger:

  • merges the new bindings into the logger's bindings (children inherit the method through the prototype chain and merge into their own bindings),
  • mirrors the new bindings into _logEvent.bindings so browser.transmit receives them, matching what child() does,
  • rebuilds the level functions (same mechanism as the level setter) so subsequent log calls prepend the updated bindings.

Consistent with Node semantics: like asChindings in lib/proto.js, updating a parent's bindings does not retroactively affect already-created children.

Testing

New test/browser-set-bindings.test.js (tape, same style as the other browser tests): method exists, bindings appear in subsequent logs, repeated calls merge, child loggers merge with their own bindings, works with customLevels (the issue's repro), and bindings are transmitted. All existing browser tests and lint pass.

@ankit1324

Copy link
Copy Markdown
Contributor Author

For transparency: after opening this I noticed #2403 tackles the same issue and predates this PR. I'd normally close mine in favor of the earlier one, but the implementations differ in ways that matter, so leaving this open for the maintainers to pick:

Happy to close either way if the maintainers prefer building on #2403.

@ankit1324

Copy link
Copy Markdown
Contributor Author

Re the red CI: the failing test (thread-stream async flush should call the passed callback) fails identically on a clean main checkout — it's unrelated to this PR's browser.js changes. I bisected it to the thread-stream 4.1.0 release floating in via the ^4.0.0 range and filed #2473 with details (last green main CI was 2026-05-08, right before 4.1.0 was published). The browser tests here all pass.

@mcollina

Copy link
Copy Markdown
Member

Please rebase

The browser logger did not implement setBindings even though the
types declare it on LoggerExtras, so calling logger.setBindings()
in a browser build threw "setBindings is not a function".

Merge the new bindings into the logger's bindings, mirror them into
_logEvent for transmit, and rebuild the level functions so subsequent
log calls prepend the updated bindings.

Fixes pinojs#2021
@ankit1324
ankit1324 force-pushed the fix/browser-set-bindings branch from fa07b69 to 91fb6bb Compare July 13, 2026 15:07
@ankit1324

Copy link
Copy Markdown
Contributor Author

Rebased on main (now includes the #2470 flush-test fix). Browser tests and the previously failing transport test both pass locally.

@ankit1324

Copy link
Copy Markdown
Contributor Author

@mcollina Rebase done and CI is green now (14 passing, the transport failure is gone with #2470 on main). Ready for review whenever you get a chance — happy to address any feedback.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
mcollina merged commit d514a8a into pinojs:main Jul 18, 2026
15 checks passed
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.

[bug] logger.setBindings undefined when using esm exported pino

2 participants