New TypeScript types for logging reject many valid interpolation inputs

[mdekstrand]

The new TypeScript types introduced in #2230 are much more strict than the functions accept, causing many false-positive typing errors in TypeScript code using Pino. #2259 is one example, but the overstrictness seems to go much farther.

• The %s specifier in util.format will call obj.toString() if the object provides a toString() method, but the new typing rejects such objects as input to %s. Arbitrary objects with toString() are very useful when logging custom objects that have a natural string representation, such as a path or URL object.
• %O and friends accept undefined (printing undefined) and null, but neither is allowed by the TypeScript types.

As near as I can tell, any JavaScript type seems to be valid input to %s, %O, and %o in util.format, so it seems that any would be a better input type for these format specifiers.

The result of this overstrictness is that code that type-checked prior to 9.8, and runs fine, now fails to typecheck with numerous errors (184 errors in 10K SLOC in one of my projects, that are spurious as far as I can tell).

[akselinurmio]

I'm wondering why the breaking change for types wasn't included in a major version update? I understand the project follows semantic versioning, so I wanted to clarify the reasoning since breaking changes are typically reserved for major version bumps.

[snrmwg]

That's why I stay on v9.7.0 for now. Way too strict. Maybe a separate VeryStrictLogger would have made sense.

[kibertoad]

I hope that the new types can be improved without fully reverting to the old behaviour, as it was definitely way too loose. We have definitely caught a lot of logging misuse after migrating to 9.9.0, and none of our valid cases were affected.
(and yeah, making it a semver major and just saying that new behaviour is how it's going to be from now on might have been a win-win approach)

[mdekstrand]

@kibertoad would relaxing the types for %s, %o, and %O to any break many of those detected misuses? %o and %O are clearly any, as their purpose is to inspect any object; %s also has defined behavior for any JavaScript input object (although the util.format behavior is a little weird, if the object doesn't override Object.prototype.toString, it uses util.inspect like %o instead of the default toString). I think that mostly leaves %d as a type-checkable parameter though.

Further, there isn't a different interpolator for toString behavior than %s, so if what the logging code wants is “call val.toString() like %s”, and %s is restricted to strings, they don't have another type-compatible option.

[mcollina]

9.9.1 should address most of it. Is there anything missing @mdekstrand @snrmwg ?

[mdekstrand]

@mcollina That fixes several of them, but a couple of other false positive categories in my code:

• passing undefined to %O or %o
• objects implementing toString() passed to %s

[kibertoad]

objects implementing toString() passed to %s

I don't think there is a reasonable way to support this case without also breaking important type-checking for invalid arg order (params object after message string). In this case just call toString() and that should be it.

[mdekstrand]

Yes, thinking through this the following goals seem mutually incompatible, at least at first:

1. Making the types accept any object accepted by the format string code.
2. Detecting when the caller has put the string and params in the wrong order.

However, the individual argument types are not the only error detection. The detailed typing for the log messages also types the number of arguments, even if individual arguments have the any type. So a misordered call like:

log.info("hello %s", {params: "misplaced"}, "world")

can still fail typechecking, even if %s is typed as any, because there are too many params: it will be trying to coerce [{params: string}, string] to [any], which is an incorrect type.

Typing %s as any would not catch a line that simultaneously puts parameters and message in the wrong order and omits the value to interpolate into the message, however.

For the %s/toString() case, manually calling toString() works, but I'm not excited about it for a couple of reasons:

• It's a lot of changes (at least 100 in under 10k SLOC in my project that's most directly affected).
• In the case where toString() is expensive (not a problem in my current code, but thinking generally), calling toString() at the point of call means it is always called, instead of allowing the logger to skip the call if the log message will not be emitted.

For %o/%O, I don't see a very good solution for typing them as anything other than any, because the purpose of those format codes is to inspect literally any object. To have them typed as something other than any is to declare at the type layer that no, they do not serve the purpose they are documented to fulfill in util.format.

[mcollina]

Here is an example:

import { pino } from './pino.js';

const log = pino()

log.info('Hello world %s', { foo: 'bar' });

const foo = {
  bar: 42,
  toString () {
    return '<<foo>>'
  }
}

log.info('Hello world %s', foo);

The first one is a mistake, and the second one is correct:

{"level":30,"time":1757086859317,"pid":67181,"hostname":"Matteos-MacBook-Pro.local","msg":"Hello world [object Object]"}
{"level":30,"time":1757086859317,"pid":67181,"hostname":"Matteos-MacBook-Pro.local","msg":"Hello world <<foo>>"}

We need to find a way to allow the second one, but keep blocking the first one.

[mcollina]

I think this is the last one on this change: #2277.

[snrmwg]

9.9.1 should address most of it. Is there anything missing @mdekstrand @snrmwg ?

I tried 9.9.4 and still get a lot of type errors. Changing “%s” to “%o” in the msg string does not fix the errors. I also don't see any advantage in using val?.toString() for arguments. Not only does this worsen the structure of the code, but it can also have a negative impact on performance, as @mdekstrand has already pointed out. I'll stick with 9.7.0 for now.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.