Partial override of default configuration not available when using TypeScript

[nullromo]

Because the typing for DailyRotateFileTransportOptions includes RequireOnlyOne<GeneralDailyRotateFileTransportOptions, 'filename' | 'stream'>, TypeScript will complain if neither a filename nor a stream is specified in the options object.

In the README, filename is listed as having a default value of 'winston.log.%DATE% (which I'm pretty sure is a typo [no closing single-quote] anyway), and stream is listed as default null. From reading the README, I expect that creating a transport like this:

winston.createLogger({
    transports: [
        new DailyRotateFile({
            dirname: 'logs',
        }),
    ],
});

will work, allowing me to use the default filename option with my own custom dirname option. However, this does not work. TypeScript says that the type { dirname: string; } is not assignable to the parameter. Fixing it like this:

new DailyRotateFile({
    filename: 'winston.log.%DATE%',
    dirname: 'logs',
}),

works, of course, but in the absence of a custom filename, the default should be allowed, even with other custom options like dirname.

[mattberther]

@nullromo Would you please take a look at #307 and let me know if that will solve this issue for you. Also, as Im not familiar with TypeScript, I'll see if @Slessi is also available for a quick sanity check on the approach in that PR before I merge.

[nullromo]

@mattberther That idea seems to work. I tried out the code and it worked for my purposes, however I did not extensively test it. I also created another pull request #308 with a minor edit. Thank you for looking into this!

[Slessi]

This would be a regression.

See relevant conversation https://github.com/winstonjs/winston-daily-rotate-file/pull/238#discussion_r324716271 And TS playground test here

Original situation achieves its intended goal, one of stream/filename is required (I realise this disagrees with your situation)

With this change, the situation is worse however. It is allowed to pass neither, but now also allowed to pass both, and the function isnt automatically aware of either parameter.

[nullromo]

Ah yes, this change does mean const a: Options = {filename: '', stream: ''} stops giving an error, which is not acceptable. I am fairly new to TypeScript myself. Perhaps this question will yield a solution.

[Slessi]

@nullromo probably all that is required here is modifying RequireOnlyOne by wrapping the following:

{
    [K in Keys]-?:
        Required<Pick<T, K>>
        & Partial<Record<Exclude<Keys, K>, undefined>>
}[Keys]

with Partial<>

Partial<{
    [K in Keys]-?:
        Required<Pick<T, K>>
        & Partial<Record<Exclude<Keys, K>, undefined>>
}[Keys]>;

Please test properly

Or maybe you can just drop the Required<> around that Pick and u wont need the partial

[nullromo]

I submitted #309, which provides a working solution and is a little less verbose.

[Slessi]

@nullromo do as I suggested above ^ it still doesnt work

[nullromo]

I botched it with a typo again. This is what I meant to do (see lines 14 and 15). I updated the PR.

[Slessi]

@nullromo My personal opinion is that you should still just remove Required from the Pick. There are many new unneeded types created in your PR, for a simple 1 line change.

[nullromo]

The PR is updated.

[Slessi]

kinda confusing which PR to check but #309 seems ok

[mattberther]

Resolved with #307 and pushed to npm as winston-daily-rotate-file@4.5.1.