thelicato / pino-rotating-file-stream

A transport for pino that rotates logs
https://www.npmjs.com/package/pino-rotating-file-stream
MIT License
5 stars 1 forks source link

Remove defaults #1

Open marchaos opened 8 months ago

marchaos commented 8 months ago

Hey. In https://github.com/thelicato/pino-rotating-file-stream/blob/main/src/index.ts you set some default values, but in my case I don't want to have an interval, but with those I cannot set it to undefined.

Are you able to remove those default values so that those can be undefined?

thelicato commented 8 months ago

Hello, the interface PinoRotatingFileStreamOptions extends the Options interface defined in the rotating-file-stream package. It should be defined here: https://github.com/iccicci/rotating-file-stream/blob/master/index.ts#L55

Since the values you are referring to are optional I think you can manually set them to undefined/none. I haven't tested it out, so please let me know if it works.

longzheng commented 1 month ago

I'm also running into this problem with compress: false which doesn't work due to the defaults. (Sorry compress is a bad example since false isn't supported by rotatingfile-stream`)

Hello, the interface PinoRotatingFileStreamOptions extends the Options interface defined in the rotating-file-stream package. It should be defined here: https://github.com/iccicci/rotating-file-stream/blob/master/index.ts#L55

Since the values you are referring to are optional I think you can manually set them to undefined/none. I haven't tested it out, so please let me know if it works.

This doesn't work because the defaults are using || which means a falsy value of undefined or false will all fallback to the specified defaults

https://github.com/thelicato/pino-rotating-file-stream/blob/53a9d14a3de41ff303f6e99aba1a418f95af82e4/src/index.ts#L13-L16

For example

// assume interval = undefined

undefined || '7d'
// '7d'
longzheng commented 1 month ago

Sorry actually looking at the rotating-file-stream library, it looks like they actually don't accept undefined for a lot of values, including interval

Error: Don't know how to handle 'options.interval' type: undefined
    at Object.interval (C:\Users\longz\Documents\GitHub\pino-rotating-file-stream\node_modules\rotating-file-stream\dist\cjs\index.js:438:19)
    at checkOpts (C:\Users\longz\Documents\GitHub\pino-rotating-file-stream\node_modules\rotating-file-stream\dist\cjs\index.js:535:20)
    at createStream (C:\Users\longz\Documents\GitHub\pino-rotating-file-stream\node_modules\rotating-file-stream\dist\cjs\index.js:579:18)

I'm not sure why they made the type optional

https://github.com/iccicci/rotating-file-stream/blob/e4a46274e672920fcb38b0cdf65ddc3e2ea7da6c/index.ts#L62

but then don't actually support undefined

https://github.com/iccicci/rotating-file-stream/blob/e4a46274e672920fcb38b0cdf65ddc3e2ea7da6c/index.ts#L601

With that in mind then, I guess this library always setting some defaults even for falsy values makes sense.