createLogger breaking changes under patch release

[rossanmol]

Please tell us about your environment:

• winston version?
  • [+] winston@3
• node -v outputs:
• Operating System? macOS
• Language? Typescript 3.5.3

What is the problem?

createLogger exported by winston, will now return Logger<winston.AbstractConfigSetLevels> which is a breaking change as before 3.3.0/3.3.1 release createLogger would return Logger.

What do you expect to happen instead?

Breaking change is introduced in a major version

Other information

n/a

[rossanmol]

Looks like the following PR actually broke createLogger: https://github.com/winstonjs/winston/pull/1807/files

[DABH]

Will investigate but would love some help from TS-savvy people. Logger<T> has a default template parameter, so if you just write Logger shouldn't it translate to Logger<DefaultLevels> for you and not cause issues? Similarly createLogger should return Logger<DefaultLevels> by default, can that not be used as Logger?

If not, maybe the simplest solution -- since #1807 was a real bug fix -- is to add some export in index.d.ts like export Logger = Logger<DefaultLevels>;? (Not sure of exact syntax, maybe you would know?) PR would be welcomed.

[Pigotz]

Hi @DABH 👋

It looks like somebody forgot a generic inside the Logger class type. I just opened a PR in case you want to give it a look!

[DABH]

@Pigotz 's fix will make createLogger return a Logger<T> instead of a Logger<AbstractConfigSetLevels>. This is definitely a fix but I'm not sure if it will allow using just Logger as the typename or if one is still required to write Logger<T>. Maybe @Pigotz or @rossanmol one of you can give this a try and let me know so we can be sure to fix this properly?

[DABH]

cc @Mizumaki

[rossanmol]

Can confirm that the fix solves the problem, and createLogger can now be called with/without specifying generic.

However, there is another breaking change with this change where consumers could previously pass levels as Dictionary<number> or Record<string, number> but now are restricted by DefaultLevels.

[rossanmol]

The generic should be T extends Config.AbstractConfigSetLevels = DefaultLevels | Config.AbstractConfigSetLevels to avoid breaking v3

[Mizumaki]

@DABH Sorry to commit code with some mistakes... 😭
@Pigotz Thanks for fixing it! 👍

@rossanmol
Hmm, sorry but I can't figure out what problem occurred.
Do you face some concrete problems because of this change, or you wanna just mention this is breaking change? If you have some problem, can you explain it in detail?

In my environment, T extends Config.AbstractConfigSetLevels = DefaultLevels is enough and T extends Config.AbstractConfigSetLevels = DefaultLevels | Config.AbstractConfigSetLevels doesn't make any difference. (because if you omit the generics, always DefaultLevels is set.)

Anyway, sorry to commit the breaking change in patch release. 🙇

[Mizumaki]

However, there is another breaking change with this change where consumers could previously pass levels as Dictionary or Record<string, number> but now are restricted by DefaultLevels.

Ahhhh, maybe I understand.

This is your problem. right ?
If you type Logger directly, there is an error occurred.

Wait for me to fix it.
really sorry

[rossanmol]

Here is a sample code, with the new problem (with the changes of #1817):

As you can see, DefaultLevels is too strict compared to what we had before.

[AuHau]

Also, I believe that the methods that return another Logger instance should use the "parent's" generic.

E.g. currently child is typed as child(options: Object): Logger. I would argue it should be child(options: Object): Logger<T>.

[sigorilla]

@DABH Can you create new version with this fix (bc6f681)?

[DABH]

@sigorilla , I'm waiting for the fixes/changes proposed by @AuHau @Mizumaki @rossanmol . Will give those folks a few hours -- I will check back in a few hours and hopefully we can get a proper fix merged and released then. Thanks all.

[Mizumaki]

@DABH In my conclusion, #1807 can't exist without breaking change.
So, I suggest reverting commits...

#1820

If anyone can fix the problem without breaking change, please take over my PR
cc @AuHau @rossanmol .

[DABH]

Thanks again all, for now we will just revert and ship this as v3.3.3. I'm definitely still interested in getting a working/backwards-compatible version of this working, if people are still interested in pursuing this. Thanks again!