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

[Bug]: 'level' parameter of winston.createLogger() no longer accepts type 'string', breaks ability to set log level via environment variable #2047

Open
bradenmitchell opened this issue Jan 28, 2022 · 8 comments · May be fixed by #2048

Comments

@bradenmitchell
Copy link

The problem

When using environment variables to set 'level' property of winston.createLogger({ level: ... }), VSCode displays the following error: Type 'string' is not assignable to type 'level | undefined'.

It appears that in v3.5.0 the type for the level parameter was changes from 'string | undefined' (as in v3.4.0 and prior) to 'level | undefined' where 'level' is a type defined by winston as type level = "debug" | "error" | "warn" | "info" | "http" | "verbose" | "silly"

Even when defining the type for the environment variable to winston.level this type error still occurs:

import { level } from 'winston'
declare namespace NodeJS {
  interface ProcessEnv {
    LOG_LEVEL: level
  }
}

then

import { createLogger } from 'winston'
const logger = createLogger({
  level: process.env.LOG_LEVEL  // error occurs here
})

Current workaround

import level and assert the environment variable as the level type.

import { createLogger, type level } from 'winston'

const logger = createLogger({
  level: process.env.LOG_LEVEL as level
})

What version of Winston presents the issue?

3.5.0

What version of Node are you using?

14.17.3

If this worked in a previous version of Winston, which was it?

3.4.0

Minimum Working Example

No response

Additional information

No response

@DABH
Copy link
Contributor

DABH commented Jan 28, 2022

This looks like potentially an unintended breaking change but arguably stronger typings are enforced now? What do you think is the best solution? Add some warning to the CHANGELOG? Make the type level | undefined | string?

@nathanbrock
Copy link

@DABH - Agreed, the stronger enforced typging is great, but it's unfortunatly blocking automated pipelines. Does the project consider this a breaking change and therefore something to be included in a subsequent major version bump? That said, certainly wouldn't want to slow down progress 😅 Your suggestion of making the type level | undefined | string would work nicely with a deprecation notice included in the CHANGELOG?

(Thanks for raising this issue, @bradenmitchell)

@IlyaKhD
Copy link

IlyaKhD commented Jan 28, 2022

What is more: using custom levels is now broken at compile time (TypeScript):

const levels = {
  foobar: 99,
  warn: 2,
  error: 1,
};

const logger = createLogger({
  levels,
});

const level: keyof typeof levels = 'foobar';

logger.log(level, 'message'); // Argument of type '"foobar"' is not assignable to parameter of type 'level'.ts(2769)

Due to level: string was changed to level: level here:

winston/index.d.ts

Lines 79 to 85 in 3b21cc4

interface LogMethod {
(level: level, message: string, callback: LogCallback): Logger;
(level: level, message: string, meta: any, callback: LogCallback): Logger;
(level: level, message: string, ...meta: any[]): Logger;
(entry: LogEntry): Logger;
(level: level, message: any): Logger;
}

Maybe, adding generic parameter would resolve the issue.

@DABH
Copy link
Contributor

DABH commented Jan 28, 2022

PR welcomed from whoever wants to implement the desired fix here!

carboneater pushed a commit to carboneater/winston that referenced this issue Jan 28, 2022
…m values

Fixes winstonjs#2047

BREAKING CHANGE: v3.5.0 introduced typings changes that broke typings for  custom (non-npm) levels
Generous application of Generics allowed to circumvent the original issues, whilst maintaining typings improvements
Default levels should be unaffected.
Integrations using custom log levels, and the Logger interface will have to add a type argument of their interfaces.
@carboneater
Copy link

So I opened #2048 to apply generics most everywhere to solve the overly strict types.

However, for all the flexibility & ease of use I've worked into that one, it is still not globally without breaking changes.
(Typescript users using custom loglevels will need to tweak their type annotations.)

Going forward, I would strongly advise reverting #1896 and publishing it as 3.5.1.

#1896 & #2048 should be released as part of a new major version, if only because they can break TypeScript code using Winston.

@ghost
Copy link

ghost commented Jan 31, 2022

Why aren't the syslog levels included in the stronger typed level type? Kind of odd that there are syslog methods with the interface LeveledLogMethod but that it's level values are not included in the level type.

@wbt
Copy link
Contributor

wbt commented Jan 31, 2022

Going forward, I would strongly advise reverting #1896 and publishing it as 3.5.1.

This has been done. Apologies for missing the impact of adding those types on custom levels!
Also tests involving custom levels which would've caught that earlier are welcome.

@carboneater
Copy link

Also tests involving custom levels which would've caught that earlier are welcome.

AFAIK, #2048 should also provide some typings validation for custom types using winston

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants