Enhance logger function typing #2230

samchungy · 2025-06-11T09:53:16Z

An even bigger upgrade to #1995

This is a breaking change to the types, improving type safety by enforcing stricter parameter typing on all logger methods.

Existing code that passes metadata after the message string will need to be updated. This pattern was previously silently ignoring the metadata, but now triggers a type error to prevent data loss.

Before (no longer works):

logger.error('my message', { err, metadata });
logger.error('my message', 'extra metadata');

After (correct usage):

logger.error({ err, metadata }, 'my message');
logger.error({ extra: 'extra metadata' }, 'my message');

This ensures all metadata is properly captured and logged.

This update also enforces type checking for placeholder values in message strings:

Before (no longer works):

logger.error('error message: %s code: %d', 400, 'Bad Request');
logger.error({ err, metadata }, 'error message: %s code: %d', 400, 'Bad Request');

After (correct usage):

logger.error('error message: %s code: %d', 'Bad Request', 400);
logger.error({ err, metadata }, 'error message: %s code: %d', 'Bad Request', 400);

samchungy · 2025-06-11T11:45:11Z

test/types/pino.test-d.ts

+info('The json is %j', { a: 1, b: '2' });
+info('The object is %O', { a: 1, b: '2' });
+info('The answer is %d and the question is %s with %o', 42, 'unknown', { correct: 'order' });
+info('Missing placeholder is fine %s');


I've allowed strings with missing placeholder data but if we want to be more strict and require that they add them we can just change the type of args from ...args: ParseLogFnArgs<TMsg> | [] to ...args: ParseLogFnArgs<TMsg> .

But, this is technically valid

tadhglewis · 2025-06-11T23:53:08Z

lgtm 🚀

mcollina

wow. looks really good!

samchungy · 2025-06-15T10:30:12Z

In case you want something for a changelog, we wrote something like this for ours. Feel free to throw it away/tweak it however you would like @mcollina 🙏

'@seek/logger': major

Enforce stricter typing for logger methods

This is a breaking change to the types, improving type safety by enforcing stricter parameter typing on all logger methods.

Existing code that passes metadata after the message string will need to be updated. This pattern was previously silently ignoring the metadata, but now triggers a type error to prevent data loss.

Before (no longer works):

logger.error('my message', { err, metadata });

After (correct usage):

logger.error({ err, metadata }, 'my message');

This ensures all metadata is properly captured and logged.

This update also enforces type checking for placeholder values in message strings:

Before (no longer works):

logger.error({ err, metadata }, 'my message error code: %d', 'Bad Request');

After (correct usage):

logger.error({ err, metadata }, 'my message error code: %d', 400);

samchungy · 2025-06-16T00:16:42Z

One small side-effect of this which someone in my team ran into is if you're trying to access the calls via a Jest Mock - you may lose typing from the 2nd argument onwards due to the conditional types that I've added.

Eg.

logger.warn.mock.calls[0]?.[1]

You may need to change that to:

logger.warn.mock.calls[0]?.at(1)

dcitron · 2025-08-08T19:59:05Z

@samchungy we noticed an oddity -- we're using a logMethod hook as suggested in the documentation -- https://getpino.io/#/docs/api?id=message

The hook allowed us to write code like this:

// ...
  } catch (err) {
    pino.error(getContext(), 'Failed to send error to Slack:', err);
  }

And also this:

console.info = (...args) => {
  pino.info(getContext(), ...args);
};

After this change to Pino, it seems that now we have to define a type that represents the prior definition of logFn and use it like this:

type UnsafePinoLogFn = (obj: unknown, msg?: string, ...args: unknown[]) => void;

// ...
  } catch (err) {
    (pino.error as unknown as UnsafePinoLogFn)(
      getContext(),
      'Failed to send error to Slack:',
      err
    );

// ...

console.info = (...args) => {
  (pino.info as unknown as UnsafePinoLogFn)(getContext(), ...args);
};

// ... etc

Does that seem to make sense?

samchungy · 2025-08-08T22:23:25Z

Is getContext() returning a placeholder string? Otherwise err would be getting lost to the void.

This change is bound to get people coming here, but unless you're using a placeholder string, Pino actually drops everything after a message string.

I would suggest something like:

pino.error({ ...getContext(), err }, 'Failed to send error to Slack:');

dcitron · 2025-08-08T23:29:54Z

Oh, getContext() is returning an object.

So, maybe this is unorthodox, but we're monkeypatching console.error etc with pino methods, so as a result, we have a logMethod hook that supports multi-argument logging with formatting placeholders and error/object handling the way console.log does.:

/**
 * A custom logMethod hook for Pino that mimics console.log behavior
 * for handling multiple arguments beyond the first input.
 */
const consoleLogPinoHook = {
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  logMethod(args: any[], method: (...args: any[]) => void) {
    if (args.length <= 2) {
      // Default behavior for a single argument
      method.apply(this, args);
      return;
    }

    // Handle multiple arguments like console.log does
    // Format: first argument is the message (potentially with format specifiers)
    // followed by any number of arguments to be substituted or concatenated
    const contextArg = args.shift();
    const firstArg = args[0];
    const restArgs = args.slice(1);

    // If the first argument is a string and contains format specifiers,
    // handle them similar to how console.log would
    { ... }
    
    // Else no format specifiers or first arg is not a string
    // Just concatenate everything with spaces, like console.log does
    { ... }

  },
};

samchungy · 2025-08-09T04:26:37Z

The change I made where was to help prevent users from sending their metadata to the void for a logger with standard configuration.

I think defining something custom would be inevitable for your case

You can restore the previous types with an overload/declaration merging

Eg.

declare module "pino" {
  interface LogFn {
    <T extends object>(obj: T, msg?: string, ...args: any[]): void;
  }
}

I would keep this next to your monkey patching code.

dcitron · 2025-08-09T15:09:34Z

That makes sense! Thank you!

Dhruv-Garg79 · 2025-08-09T19:05:48Z

I am also facing type error after recent update. The below code used to work earlier.

class Logger {
	private readonly requestInfo: TRequestInfo;

	constructor(reqInfo: TRequestInfo) {
		this.requestInfo = reqInfo;
	}

	public debug(msg: string, ...args: any[]): void {
		pinoLogger.debug(this.requestInfo, msg, ...args);
	}
}

If I change the new type definition to have ...args: any[] instead of ...args: ParseLogFnArgs<TMsg> | []. It will start working for me again. But, is there a better way to do it?

interface LogFn {
        <T, TMsg extends string = string>(obj: T, msg?: T extends string ? never: TMsg, ...args:any]): void;
        <_, TMsg extends string = string>(msg: TMsg, ...args: ParseLogFnArgs<TMsg> | []): void;
    }

samchungy · 2025-08-10T01:04:38Z

I am also facing type error after recent update. The below code used to work earlier.

class Logger {
	private readonly requestInfo: TRequestInfo;

	constructor(reqInfo: TRequestInfo) {
		this.requestInfo = reqInfo;
	}

	public debug(msg: string, ...args: any[]): void {
		pinoLogger.debug(this.requestInfo, msg, ...args);
	}
}

If I change the new type definition to have ...args: any[] instead of ...args: ParseLogFnArgs<TMsg> | []. It will start working for me again. But, is there a better way to do it?

interface LogFn {
        <T, TMsg extends string = string>(obj: T, msg?: T extends string ? never: TMsg, ...args:any]): void;
        <_, TMsg extends string = string>(msg: TMsg, ...args: ParseLogFnArgs<TMsg> | []): void;
    }

Before you consider changing the types, are you also similarly patching Pino? Have you read why the change is happening? Are you aware args aren't being logged unless msg is a placeholder string?

AaronMoat · 2025-08-10T02:59:57Z

@Dhruv-Garg79 if I'm understanding your need correctly, you could make the debug method also use the new generics:

class Logger {
	private readonly requestInfo: TRequestInfo;

	constructor(reqInfo: TRequestInfo) {
		this.requestInfo = reqInfo;
	}

	public debug<TMsg extends string = string>(
        msg: TMsg,
        ...args: pino.ParseLogFnArgs<TMsg>
    ): void {
		pinoLogger.debug(this.requestInfo, msg, ...args);
	}
}

This allows you to have type safety in your custom class.

Playground

Dhruv-Garg79 · 2025-08-11T08:32:48Z

Before you consider changing the types, are you also similarly patching Pino?

No

Have you read why the change is happening?

yes, but it will lead to lot of changes for us, that's why I asked if there was any other better way to handle it instead of just monkey patching types.

Are you aware args aren't being logged unless msg is a placeholder string?

yes. and that doesn't affect us. since we are using a small wrapper over pino.

Hey @AaronMoat, that's a good idea, but that will require us to make a lot of changes wherever we are using the logger. Because value can be T or undefined where %s or %j is used in our case.

AaronMoat · 2025-08-11T09:13:51Z

Hi @Dhruv-Garg79 — are you able to show a problematic TS Playground example? I'm not following the problem, nor what T is in your comment.

Dhruv-Garg79 · 2025-08-11T13:47:24Z

@AaronMoat I have updated your TS playground with the issue. The problem arises when the templated value is null or undefined.

playground

samchungy · 2025-08-11T14:59:00Z

I mean I think that makes sense there, you're trying to pass something that could be null or undefined to a placeholder which is expecting a string.

You can always bypass it with some casts or coercion.

Either:

l.debug('hello %s', value ?? '');

l.debug('hello %s', value as string);

Or using any on your own logger implementation. Alternatively, write your own version of ParseLogFnArgs which accepts null or undefined and use a cast to pass it to pino.

eg. pinoLogger.debug(this.requestInfo, msg, ...args as ParseLogFnArgs<TMsg>);

	public debug<TMsg extends string = string>(
        msg: TMsg,
        ...args: any
    ): void {
		pinoLogger.debug(this.requestInfo, msg, ...args);
	}

Or declare the override as suggested above.

clemens · 2025-08-11T18:49:31Z

At the risk of asking a stupid question: Why is this done as part of a minor release when it clearly breaks the public API (even if it's "only" the types) of a lot of functions on the main/only object? And if we say this is acceptable, shouldn't the changelog at least mark it as a breaking change (instead, it's even called "enhance", which suggests an improvement)?

Dhruv-Garg79 · 2025-08-12T04:46:00Z

@samchungy I get it, but since it's logging. I think we should not need to handle it by casting or coercion. For now, I can override the types, but it should allow null/undefined directly.

samchungy · 2025-08-12T05:09:37Z

I think we should not need to handle it by casting or coercion

We can agree to disagree but I think if your template is expecting a string, the type should show that.

Unless you were to introduce alternate identifiers eg. %ns (nullish string?), I think the types should reflect what the templates identifiers represent.

How would you coerce null or undefined into %d which is expecting a number?

That being said, go with anys if you don't want types.

Dhruv-Garg79 · 2025-08-12T11:55:08Z

We can agree to disagree but I think if your template is expecting a string, the type should show that.

yes we can. It should be use case dependent, and the thing we are talking about is logging right now. All types, according to me should allow null/undefined by default.

Unless you were to introduce alternate identifiers eg. %ns (nullish string?), I think the types should reflect what the templates identifiers represent.

Types like %ns, %nd, %nj are a good middle ground for nullish types, which allow both null and undefined values.

I have changes my types to be like this for now

type PlaceholderTypeMapping<T extends pino.PlaceholderSpecifier> = T extends 'd'
	? number | null | undefined
	: T extends 's'
		? string | null | undefined
		: T extends 'j' | 'o' | 'O'
			? object | null | undefined
			: never;

Also shouldn't we have a separate type for boolean as well? something like %b?

samchungy · 2025-08-13T06:01:36Z

All types, according to me should allow null/undefined by default.

I think TypeScript largely disagrees with you 🤷

Also shouldn't we have a separate type for boolean as well? something like %b?

This is open source, so PR away my friend.

Dhruv-Garg79 · 2025-08-13T13:36:01Z

I think TypeScript largely disagrees with you 🤷

You took my statement out of context. I said for logging, of course, not for everything.

What I wrote was - yes we can. It should be use case dependent, and the thing we are talking about is logging right now. All types, according to me should allow null/undefined by default.

This is open source, so PR away my friend.

Sure.

klippx · 2025-08-14T08:44:02Z

At the risk of asking a stupid question: Why is this done as part of a minor release when it clearly breaks the public API (even if it's "only" the types)

It is not only the types, this is a super breaking change that disagrees with everything stated in the docs: https://getpino.io/#/docs/api?id=logging-method-parameters

This is pretty common usage in our code base:

      this.logger.warn('Got fallback values from REDA', {
        pno12: body.REDAIndividualValuesRequest.Order.PNO12,
        options: body.REDAIndividualValuesRequest.Order.Options,
        clientName: clientName ?? 'unknown',
      });

It is used as per docs, but after this PR it is "wrong" according to types (but it works in runtime)

How was this approved and merged? I don't even... 🤷

samchungy · 2025-08-14T08:49:13Z

At the risk of asking a stupid question: Why is this done as part of a minor release when it clearly breaks the public API (even if it's "only" the types)

It is not only the types, this is a super breaking change that disagrees with everything stated in the docs: https://getpino.io/#/docs/api?id=logging-method-parameters

This is pretty common usage in our code base:
      this.logger.warn('Got fallback values from REDA', {
        pno12: body.REDAIndividualValuesRequest.Order.PNO12,
        options: body.REDAIndividualValuesRequest.Order.Options,
        clientName: clientName ?? 'unknown',
      });
It is used as per docs, but after this PR it is "wrong" according to types (but it works in runtime)

How was this approved and merged? I don't even... 🤷

Well the metadata there doesn't get logged 🤷‍♂️ (have you checked your logs?)

I'm not disagreeing that it should be a breaking change

klippx · 2025-08-14T08:51:56Z

Then at the docs needs an urgent update. But it would have been great if this was a v10.0.0 release for everyone's sanity, it's kind of hard to patch the docs now. Usually it is preferred to have a version selector for the docs, now they change in the middle of v9.

But it seems this change is legit then and that I might go through with the upgrade. Wishing there was a codemod for this :)

samchungy · 2025-08-14T08:52:57Z

Then at the docs needs an urgent update. But it would have been great if this was a v10.0.0 release for everyone's sanity, it's kind of hard to patch the docs now. Usually it is preferred to have a version selector for the docs, now they change in the middle of v9.

But it seems this change is legit then and that I might go through with the upgrade. Wishing there was a codemod for this :)

I didn't write the docs but where does it say that's valid?

klippx · 2025-08-14T08:59:05Z

Ouf.... I guess I can't read - apologies. You are right. The docs seem to agree more with this PR and you. 🙇

samchungy · 2025-08-14T09:01:09Z

Ouf.... I guess I can't read - apologies. You are right. The docs seem to agree more with this PR and you. 🙇

No probs! I knew this was gonna irk some people so I was hoping it would be a major release but I promise it was done purely out of good intentions 🙏

samchungy added 5 commits June 11, 2025 19:48

Enhance logger function typing

6f9dd3b

Update pino.test-d.ts

15d0ee7

Adjust naming

bd993c7

Allow missing template specifiers

84c6e1d

rename to placeholder

d492f41

samchungy commented Jun 11, 2025

View reviewed changes

samchungy added 2 commits June 12, 2025 12:01

drop redundant unknown

ac10024

Switch to unknown

5acfefc

samchungy mentioned this pull request Jun 12, 2025

Enforce stricter typing for logger methods seek-oss/logger#182

Merged

address edge-case

87d1e2e

mcollina approved these changes Jun 15, 2025

View reviewed changes

mcollina merged commit 116b1b1 into pinojs:main Jun 15, 2025
12 checks passed

mdekstrand mentioned this pull request Aug 22, 2025

New TypeScript types for logging reject many valid interpolation inputs #2266

Closed

wolfy1339 mentioned this pull request Oct 4, 2025

Fix TypeScript errors probot/probot#2248

Merged

Enhance logger function typing #2230

Enhance logger function typing #2230

Uh oh!

Conversation

samchungy commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samchungy Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tadhglewis commented Jun 11, 2025

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

samchungy commented Jun 15, 2025

'@seek/logger': major

Uh oh!

samchungy commented Jun 16, 2025

Uh oh!

dcitron commented Aug 8, 2025

Uh oh!

samchungy commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcitron commented Aug 8, 2025

Uh oh!

samchungy commented Aug 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcitron commented Aug 9, 2025

Uh oh!

Dhruv-Garg79 commented Aug 9, 2025

Uh oh!

samchungy commented Aug 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronMoat commented Aug 10, 2025

Uh oh!

Dhruv-Garg79 commented Aug 11, 2025

Uh oh!

AaronMoat commented Aug 11, 2025

Uh oh!

Dhruv-Garg79 commented Aug 11, 2025

Uh oh!

samchungy commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clemens commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dhruv-Garg79 commented Aug 12, 2025

Uh oh!

samchungy commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dhruv-Garg79 commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samchungy commented Aug 13, 2025

Uh oh!

Dhruv-Garg79 commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klippx commented Aug 14, 2025

Uh oh!

samchungy commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klippx commented Aug 14, 2025

Uh oh!

samchungy commented Aug 14, 2025

Uh oh!

klippx commented Aug 14, 2025

Uh oh!

samchungy commented Aug 14, 2025

Uh oh!

Reviewers

samchungy commented Jun 11, 2025 •

edited

Loading

samchungy Jun 11, 2025 •

edited

Loading

samchungy commented Aug 8, 2025 •

edited

Loading

samchungy commented Aug 9, 2025 •

edited

Loading

samchungy commented Aug 10, 2025 •

edited

Loading

samchungy commented Aug 11, 2025 •

edited

Loading

clemens commented Aug 11, 2025 •

edited

Loading

samchungy commented Aug 12, 2025 •

edited

Loading

Dhruv-Garg79 commented Aug 12, 2025 •

edited

Loading

Dhruv-Garg79 commented Aug 13, 2025 •

edited

Loading

samchungy commented Aug 14, 2025 •

edited

Loading