importing should not have any side effects

[Zamiell]

This module works via side effects. In other words, all you have to do is require the module and everything works. However, this design goes against best practices for JavaScript, in which side-effects are considered evil.

Thoughts on refactoring this module so that it doesn't have side effects? After such a refactor, the end-user would invoke the module with something along the lines of:

import * as winstonDailyRotateFile from "winston-daily-rotate-file";

winstonDailyRotateFile.init();

[Adekoreday]

@Zamiell @mattberther

I think the problem starts from the pattern here

'use strict';

var winston = require('winston');
var DailyRotateFile = require('./daily-rotate-file');

winston.transports.DailyRotateFile = DailyRotateFile;
module.exports = DailyRotateFile;

this line exactly

winston.transports.DailyRotateFile = DailyRotateFile;

is this a possibility to move this out of the package @mattberther not really looked into this deeply this is just one observation

[Zamiell]

No, I don't think that a refactor should move it "out of the package". I think that the refactor would look like this:

'use strict';

var winston = require('winston');
var DailyRotateFile = require('./daily-rotate-file');

exports.init = () => {
  winston.transports.DailyRotateFile = DailyRotateFile;
};

Or, alternatively, as a default export:

'use strict';

var winston = require('winston');
var DailyRotateFile = require('./daily-rotate-file');

module.exports = () => {
  winston.transports.DailyRotateFile = DailyRotateFile;
};

[Adekoreday]

This looks good to me @mattberther what do you think ??

[mattberther]

I like the idea, but would prefer to do it in such a way that it doesn't break existing implementations. Based on the documentation, many people will have implemented using something like:

const transport = new (winston.transports.DailyRotateFile)({
    filename: 'logs/application-%DATE%.log',
    frequency: '1m',
});

Unfortunately, the default export you describe above results in TypeError: winston.transports.DailyRotateFile is not a constructor.

Thoughts on how to remove the side effect without breaking existing implementations?

[mattberther]

I'd welcome PRs for this as well. :)