feat!: change customPrettifiers to a Map

[Frederick888]

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] #551
[2] #550

---

This PR stacks on #551 to avoid conflicts down the line, so only the 2nd commit is actually relevant. (I'm not sure how to do cross-fork stacked PRs in GitHub...)

I guess the first commit in this PR will automatically disappear once #551 is (fast-forward?) merged. To review before that, we can use the link to the 2nd commit: 38cdaf27e1e2a1f33203aa249637e6e6d3086b78

[jsumners]

I'm not clear. What is this solving? It seems to just be making the code more complicated.

[Frederick888]

@jsumners Indeed it can be a little confusing as on the JavaScript side, not only does it solve nothing, it also introduces a breaking change. The issue actually came only from the TypeScript side. Allow me to share some context.

In #551 to solve #550, LevelPrettifierExtras and PrettifierExtras were merged. However the downside of #551 was that 'level' prettifier's additional extra attributes started leaking into other prettifiers. Practically, this means users can see them when doing auto-completion and use them where they are not available:

This was obviously not the intention of the original Record<string, Prettifier> & { level?: Prettifier<LevelPrettifierExtras> }. After doing some research, I reached the conclusion that having a separate 'level' prettifier type was simply impossible with Record or interface { ... }, and a Map had to be used instead.

So now on this PR:

If we don't want to break existing JavaScript users, I can instead allow customPrettifiers to accept both plain objects and Maps, and convert all Maps to objects in prettyFactory (options).

In index.d.ts, I can also either leave it as it is (so that it'll be a breaking change to TypeScript users only), or allow both Records and Maps (so that it won't be a breaking change, but users will have to use Maps whenever they want to use the additional extra attributes for the 'level' prettifier, which is kinda confusing imho).

Please let me know what you think :)

[jsumners]

I have no clue what all of that means. @mcollina and @kibertoad can whatever this is "fixing" be solved without the code complication?

[mcollina]

I really don't understand the problem, it's related to TS autocompletion, but I haven't really grasped why such a massive change is necessary.

[Frederick888]

Since the major concern regarding this PR seemed to be the number of changes, I have just pushed a new commit implementing what I described above, scoping the impact mainly to the TypeScript side. Now it won't break existing JavaScript users.

Regarding the motivation of the PR, maybe @FoxxMD, who's the author of #495, can chime in? Perhaps I'm just bad at explaining this.

Edit: @FoxxMD forgot to mention that this PR stacks on another. So to review changes actually in this PR only, please use e.g. https://github.com/pinojs/pino-pretty/compare/6da74ee..250d0d9

[mcollina]

I don't really understand why in TypeScript you have to use a map.

[jsumners]

This feels like a hack to me, and I don't like the idea of maintaining a hack to make some language that isn't JavaScript do things.

[FoxxMD]

I'm not entirely sure why a Map is needed and haven't looked too closely at this...but

In 551 to solve 550, LevelPrettifierExtras and PrettifierExtras were merged...This was obviously not the intention of the original Record<string, ...

It sounds like you are complicating the typings in order to fix a mistake made in the earlier PR...that you introduced. I'd suggest re-visting the changes made in the original PRs 550/551 and try to fix the root issue with another approach rather than trying to dig out of the hole you've already created.

[Frederick888]

It sounds like you are complicating the typings in order to fix a mistake made in the earlier PR...that you introduced. I'd suggest re-visting the changes made in the original PRs 550/551 [...]

I'm on my phone now and I'm not good at typing on my phone so I'll keep my comment short.

#550 is an issue, not a PR. It details a bug which I think was introduced by #495. There is a test case change in #551. If you copy the test to the main branch, it will fail, which means either the original Record approach in #495 didn't work, or I somehow used it wrong.

If I was indeed wrong, or there's a better solution, I'm all ears.

[Frederick888]

The issue with #551 is that if you write something like level123: (level, levelKey, log, { label, labelColorized, colors }) in TypeScript, it still transpiles while it shouldn't. label and labelColorized are only available to level.

But as in #550 I figured there was no way to fix all these without a breaking change, #551 itself can be released as a new patch version so at least in TypeScript, level prettifier can use those two attributes.

[jsumners]

It is not clear to me what needs to happen here. I think both @mcollina and I are asking for justification, but have not seen anything that qualifies.