Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeScript error when using PrettyOptions.customPrettifiers.level with extras #550

Open
Frederick888 opened this issue Dec 17, 2024 · 4 comments · May be fixed by #551
Open

TypeScript error when using PrettyOptions.customPrettifiers.level with extras #550

Frederick888 opened this issue Dec 17, 2024 · 4 comments · May be fixed by #551
Labels
good first issue Good for newcomers

Comments

@Frederick888
Copy link

Frederick888 commented Dec 17, 2024

For example,

function pinoPrettyTransport(opts: PrettyOptions): PrettyStream {
  return PinoPretty({
    ...opts,
    translateTime: 'UTC:yyyy-mm-dd"T"HH:MM:ss.l"Z"',
    customPrettifiers: {
      level: (_level, _levelKey, _log, { labelColorized }) => labelColorized.toLowerCase(),
    },
  })
}

...leads to

error TS2322: Type '{ level: (_level: string | object, _levelKey: string, _log: object, { labelColorized }: PrettifierExtras<LevelPrettifierExtras>) => string; }' is not assignable to type 'Record<string, Prettifier<object>> & { level?: Prettifier<LevelPrettifierExtras> | undefined; }'.
  Type '{ level: (_level: string | object, _levelKey: string, _log: object, { labelColorized }: PrettifierExtras<LevelPrettifierExtras>) => string; }' is not assignable to type 'Record<string, Prettifier<object>>'.
    Property 'level' is incompatible with index signature.
      Type '(_level: string | object, _levelKey: string, _log: object, { labelColorized }: PrettifierExtras<LevelPrettifierExtras>) => string' is not assignable to type 'Prettifier<object>'.
        Types of parameters '__3' and 'extras' are incompatible.
          Type 'PrettifierExtras<object>' is not assignable to type 'PrettifierExtras<LevelPrettifierExtras>'.
            Type '{ colors: Colorette; }' is missing the following properties from type 'LevelPrettifierExtras': label, labelColorized

115     customPrettifiers: {
        ~~~~~~~~~~~~~~~~~

It seems since Record<string, Prettifier> doesn't restrict what keys it can have, Prettifier must be assignable to Prettifier<LevelPrettifierExtras>?

I'm new to TypeScript so maybe it's me that's using it wrong. Please help, thanks!

Edit: I'm using TypeScript v4.8.4, Node.js v18.9.1.

@mcollina mcollina added the good first issue Good for newcomers label Dec 18, 2024
@mcollina
Copy link
Member

Thanks for opening a PR! Can you please add a unit test? We use tsd for types.

@Frederick888
Copy link
Author

Hi @mcollina, I'm happy to contribute a test but I'm not sure what the best way is to fix it.

The test change can be as simple as:

diff --git a/test/types/pino-pretty.test-d.ts b/test/types/pino-pretty.test-d.ts
index 9f2acd7..2f41344 100644
--- a/test/types/pino-pretty.test-d.ts
+++ b/test/types/pino-pretty.test-d.ts
@@ -32,9 +32,9 @@ const options: PinoPretty.PrettyOptions = {
   customPrettifiers: {
     key: (value) => {
       return value.toString().toUpperCase();
     },
-    level: (level, label, colorized) => {
+    level: (level, levelKey, log, { label, labelColorized, colors }) => {
       return level.toString();
     }
   },
   customLevels: 'verbose:5',

And I tried to still use Record or interface X { [key: string]: Y } however after some research, I'm under the impression that this is not possible. We essentially want to model a key to be any string but level (then intersect the Record with such a key with another Record<'level', ...>), which was discussed at e.g. https://stackoverflow.com/questions/51442157/type-for-every-possible-string-value-except; also https://stackoverflow.com/questions/57208330/extend-a-recordstring-string-with-a-different-type-of-property seems to be exactly the same problem, where the conclusion was again that it was possible to define such a type but it would lead to errors once you put it into use.

We can though, leverage a Map<'level', Prettifier<LevelPrettifierExtras>> & Map<string, Prettifier> type (full patch at the end), but this will be a breaking change.

We can also simply merge PrettifierExtras<object> and LevelPrettifierExtras into one {colors: Colorette.Colorette, label?: string, labelColorized?: string}, which is a compromise but hopefully not too confusing. Maybe we can do this in v13.0.1 and release the above Map method in v14?

Again I'm new to TypeScript and Node.js so please let me know if you have anything better in mind. I'll be happy to submit PR(s) with your guide.

Appendix: Map<'level', Prettifier<LevelPrettifierExtras>> & Map<string, Prettifier> full patch

diff --git a/index.d.ts b/index.d.ts
index 2c4c5c8..cc44241 100644
--- a/index.d.ts
+++ b/index.d.ts
@@ -182,12 +182,9 @@ declare namespace PinoPretty {
      *  // do some prettify magic
      * }
      * ```
      */
-    customPrettifiers?: Record<string, Prettifier> &
-      {
-        level?: Prettifier<LevelPrettifierExtras>
-      };
+    customPrettifiers?: CustomPrettifiers;
     /**
      * Change the level names and values to an user custom preset.
      *
      * Can be a CSV string in 'level_name:level_value' format or an object.
@@ -217,8 +214,9 @@ declare namespace PinoPretty {
   }
 
   function build(options: PrettyOptions): PrettyStream;
 
+  type CustomPrettifiers = Map<'level', Prettifier<LevelPrettifierExtras>> & Map<string, Prettifier>
   type Prettifier<T = object> = (inputData: string | object, key: string, log: object, extras: PrettifierExtras<T>) => string;
   type PrettifierExtras<T = object> = {colors: Colorette.Colorette} & T;
   type LevelPrettifierExtras = {label: string, labelColorized: string}
   type MessageFormatFunc = (log: LogDescriptor, messageKey: string, levelLabel: string, extras: PrettifierExtras) => string;
@@ -227,8 +225,8 @@ declare namespace PinoPretty {
   type PrettyFactory = typeof prettyFactory;
   type Build = typeof build;
   type isColorSupported = typeof Colorette.isColorSupported;
 
-  export { build, PinoPretty, PrettyOptions, PrettyStream, colorizerFactory, prettyFactory, isColorSupported };
+  export { build, CustomPrettifiers, PinoPretty, PrettyOptions, PrettyStream, colorizerFactory, prettyFactory, isColorSupported };
 }
 
 export = PinoPretty;
diff --git a/test/types/pino-pretty.test-d.ts b/test/types/pino-pretty.test-d.ts
index 9f2acd7..87874c1 100644
--- a/test/types/pino-pretty.test-d.ts
+++ b/test/types/pino-pretty.test-d.ts
@@ -3,8 +3,9 @@ import { expectType } from "tsd";
 import pretty from "../../";
 import PinoPretty, {
   PinoPretty as PinoPrettyNamed,
   PrettyOptions,
+  CustomPrettifiers,
   colorizerFactory,
   prettyFactory
 } from "../../";
 import PinoPrettyDefault from "../../";
@@ -12,8 +13,15 @@ import * as PinoPrettyStar from "../../";
 import PinoPrettyCjsImport = require("../../");
 import PrettyStream = PinoPretty.PrettyStream;
 const PinoPrettyCjs = require("../../");
 
+const customPrettifiers: CustomPrettifiers = new Map()
+customPrettifiers.set('key', (value) => {
+  return value.toString().toUpperCase();
+})
+customPrettifiers.set('level', (level, levelKey, log, {label, labelColorized, colors}) => {
+  return level.toString();
+})
 const options: PinoPretty.PrettyOptions = {
   colorize: true,
   crlf: false,
   errorLikeObjectKeys: ["err", "error"],
@@ -28,16 +36,9 @@ const options: PinoPretty.PrettyOptions = {
   timestampKey: "timestamp",
   minimumLevel: "trace",
   translateTime: "UTC:h:MM:ss TT Z",
   singleLine: false,
-  customPrettifiers: {
-    key: (value) => {
-      return value.toString().toUpperCase();
-    },
-    level: (level, label, colorized) => {
-      return level.toString();
-    }
-  },
+  customPrettifiers,
   customLevels: 'verbose:5',
   customColors: 'default:white,verbose:gray',
   sync: false,
   destination: 2,

@mcollina
Copy link
Member

@Frederick888 that works! Would you like to send a PR?

Frederick888 added a commit to Frederick888/pino-pretty that referenced this issue Jan 14, 2025
The fourth parameter of the 'level' custom prettifier -- extras -- used
to have a separate type as pino-pretty offered two additional attributes
to this particular prettifier.

However since it used a Record<string, T> type, TypeScript required all
values to be compatible with each other. So when users tried to use the
extras parameter for 'level', it caused

    error TS2322: Type '{ level: (_level: string | object, _levelKey: string, _log: object, { labelColorized }: PrettifierExtras<LevelPrettifierExtras>) => string; }' is not assignable to type 'Record<string, Prettifier<object>> & { level?: Prettifier<LevelPrettifierExtras> | undefined; }'.
    Type '{ level: (_level: string | object, _levelKey: string, _log: object, { labelColorized }: PrettifierExtras<LevelPrettifierExtras>) => string; }' is not assignable to type 'Record<string, Prettifier<object>>'.
        Property 'level' is incompatible with index signature.
        Type '(_level: string | object, _levelKey: string, _log: object, { labelColorized }: PrettifierExtras<LevelPrettifierExtras>) => string' is not assignable to type 'Prettifier<object>'.
            Types of parameters '__3' and 'extras' are incompatible.
            Type 'PrettifierExtras<object>' is not assignable to type 'PrettifierExtras<LevelPrettifierExtras>'.
                Type '{ colors: Colorette; }' is missing the following properties from type 'LevelPrettifierExtras': label, labelColorized

    115     customPrettifiers: {
            ~~~~~~~~~~~~~~~~~

This patch remedies this issue by merging LevelPrettifierExtras into
PrettifierExtras. These types are not exported directly, therefore users,
including those who leverage TypeScript utility types to extract these
types, should be able to upgrade directly.

Fixes pinojs#550
Frederick888 added a commit to Frederick888/pino-pretty that referenced this issue Jan 14, 2025
In [1], LevelPrettifierExtras and PrettifierExtras were merged to solve
a typing issue [2]. This was suboptimal as the additional extra
attributes that only the 'level' prettifier had were exposed to other
prettifiers too.

To allow the additional attributes while keeping 'level' prettifier's
type separate, this patch uses an intersection Map type
CustomPrettifiers instead.

This is a breaking change.

[1] pinojs#551
[2] pinojs#550
Frederick888 added a commit to Frederick888/pino-pretty that referenced this issue Jan 14, 2025
In [1], LevelPrettifierExtras and PrettifierExtras were merged to solve
a typing issue [2]. This was suboptimal as the additional extra
attributes that only the 'level' prettifier had were exposed to other
prettifiers too.

To allow the additional attributes while keeping 'level' prettifier's
type separate, this patch uses an intersection Map type
CustomPrettifiers instead.

This is a breaking change.

[1] pinojs#551
[2] pinojs#550
@Frederick888
Copy link
Author

@mcollina I've opened two PRs. #551 is the non-breaking remedy, #552 is the one that uses Map :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants