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

winston detetction not sufficient #1605

Closed
daniel-white opened this issue Mar 4, 2024 · 10 comments · Fixed by #1607
Closed

winston detetction not sufficient #1605

daniel-white opened this issue Mar 4, 2024 · 10 comments · Fixed by #1607
Labels
bug Something isn't working

Comments

@daniel-white
Copy link

Description

i'm trying to use pino over winston and I have to hack the logger instance to look not like the simpleified winston config.

  // HACK
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  (logger as any).color = undefined;

Without this, I get

X [MissingPeerError]: Missing peer dependency: winston. Please install it to use the feature.
    at ne (/Users/danielwhite/Repos/prism-service/node_modules/express-zod-api/dist/index.cjs:5:8015)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async Mr (/Users/danielwhite/Repos/prism-service/node_modules/express-zod-api/dist/index.cjs:18:1169)
    at async Nr (/Users/danielwhite/Repos/prism-service/node_modules/express-zod-api/dist/index.cjs:18:1838)

Expected

I would not expect express-zod-api to allow pino with no modifications

Reproduction

import pino, { BaseLogger, LevelWithSilent, LoggerOption } from 'pino';

export function createLogger(level: LevelWithSilent): BaseLogger {
  const options: LoggerOptions = {
    level
  };
  const logger = pino(options);
  return logger;
}

Context

  • Node.js version: 20.11.1
  • express-zod-api version: 17.2.0
@daniel-white daniel-white added the bug Something isn't working label Mar 4, 2024
@RobinTail
Copy link
Owner

how do you use that function createLogger that you made?
Please provide more context for reproducing the issue, @daniel-white

Please also read the corresponding article in Readme:
https://github.com/RobinTail/express-zod-api?tab=readme-ov-file#customizing-logger

@daniel-white
Copy link
Author

so createLogger is just a factory to encapsulate all the logging options - essentially the usage is like this

const logger = createLogger('info');
const config = createConfig({ logger }); // from express-zod-api, infers that it needs winston!

@RobinTail
Copy link
Owner

I think I'm getting now what's happening there.
Please stand by....

@RobinTail
Copy link
Owner

RobinTail commented Mar 5, 2024

I made a test for the function isSimplifiedWinstonConfig() against the pino instance as pino({level: "info"})

#1607

And that test passes. The function basically tells whether or not should winston be loaded instead.
Can not reproduce that behaviour so far.

What is your actual version of pino, @daniel-white

Could you run yarn why pino and tell me what is says?

@daniel-white
Copy link
Author

daniel-white commented Mar 5, 2024

hm interesting i'm using 8.19.0 as well. looks like a good test to have and ill keep playing on my end.

also, fwiw, i'm using typescript's strict mode, should that matter.

@RobinTail
Copy link
Owner

Are you sure you're supplying "info" to createLogger()?
Not one of these: "silent", "warn", "debug" , @daniel-white

@RobinTail
Copy link
Owner

Since when it's "debug", the test fails and that's I believe exactly the case, @daniel-white

@daniel-white
Copy link
Author

@RobinTail i was doing a bit more. i'm getting that value from envvars, i opened it up to more levels. it might have been on debug yesterday

I'm allowing

['error', 'warn', 'info', 'debug', 'trace']

@RobinTail
Copy link
Owner

Thank you @daniel-white , the issue confirmed.
I'll do my best to fix it ASAP.

RobinTail added a commit that referenced this issue Mar 5, 2024
@RobinTail
Copy link
Owner

@daniel-white ,
🚀 the issue fixed in the recently released version 17.2.1 .
Thank you for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants