Remove defaults

[marchaos]

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]

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]

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

pino-rotating-file-stream/src/index.ts

Lines 13 to 16 in 53a9d14

	size: size || '100M',
	maxSize: maxSize || '1G',
	interval: interval || '7d',
	compress: compress || 'gzip',

For example

// assume interval = undefined

undefined || '7d'
// '7d'

[longzheng]

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.

[bozzaj]

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.

This isn't quite accurate. If interval is passed as part of options, it needs to be a string. Here is the function that converts options: Options into Opts that later get used:
https://github.com/iccicci/rotating-file-stream/blob/e4a46274e672920fcb38b0cdf65ddc3e2ea7da6c/index.ts#L693

This method iterates through all the options that are passed. The for() loop starting at line 697 validates the options, but only the ones that are passed. Further down at line 709, if interval wasn't passed, other options are cleared.

Elsewhere, there are checks for when interval doesn't exist, such as here:
https://github.com/iccicci/rotating-file-stream/blob/e4a46274e672920fcb38b0cdf65ddc3e2ea7da6c/index.ts#L425

There's a big difference between an options Object of { interval: undefined } vs { } (no interval property defined). So, it is correct that rotating-file-stream defines interval as optional. The interface is about the property being optional, not defining the property as undefined which is something quite different.

@thelicato To the original issue, there is no real easy way to remove the interval default using undefined or any other value, based on the code as it exists in both this package or the rotating-file-stream package. However, if a default is still a good practice, but you want to support someone removing the default, you would likely have to allow undefined or null as a passed value, and then filter those values out of the Object that gets passed to createStream. Something like the following may work:

    ...otherOptions,
    // Set some default values if options not set to null (skip default)
    ...(size !== null && { size: size || '100M' }),
    ...(mazSize !== null && { maxSize: maxSize || '1G' }),
    ...(interval !== null && { interval: interval || '7d' }),
    ...(compress !== null && { compress: compress || 'gzip'}),
    path: path,

I'm giving an example using null as it's harder to tell the difference between a passed value of undefined and just not passing the property at all. However, if you wanted to use undefined instead, something like this might work:

   ...((!options.hasOwnProperty("interval") || interval !== undefined) && { interval: interval || '7d' }),

Basically, the interval property gets a default if the property isn't passed, or if the property IS passed and the property isn't set to undefined. If you wanted to use both null and undefined as "ignore the default" then you could use the hasOwnProperty example and change !== undefined to != null

Hope that helps!