winstonjs / winston-transport

Base stream implementations for winston@3 and up.
MIT License
64 stars 42 forks source link

Resolve circular dependency issues downstream in Winston #207

Closed Mr0grog closed 9 months ago

Mr0grog commented 9 months ago

Issue #1886 in Winston is caused by the circular dependency between index.js and legacy.js here, which is done in order to expose LegacyTransportStream as a property on TransportStream. The way this circular dependency works also causes an issue in this module if you import the legacy submodule before importing the main module:

const LegacyTransportStream = require('winston-transport/legacy');
const TransportStream = require('winston-transport');
new TransportStream.LegacyTransportStream({ transport: { log(info, cb) { cb(); }, on() {} }});
// Logs:
// new TransportStream.LegacyTransportStream({ transport: { log(info, cb) { cb(); }, on() {} }});
// ^

// TypeError: TransportStream.LegacyTransportStream is not a constructor
//     at Object.<anonymous> (/Users/rbrackett/Dev/winston-transport/testme.js:3:1)
//     at Module._compile (node:internal/modules/cjs/loader:1241:14)
//     at Module._extensions..js (node:internal/modules/cjs/loader:1295:10)
//     at Module.load (node:internal/modules/cjs/loader:1091:32)
//     at Module._load (node:internal/modules/cjs/loader:938:12)
//     at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12)
//     at node:internal/main/run_main_module:23:47

// Versus this, which works just fine when the legacy module is *not* imported first:
const TransportStream = require('winston-transport');
new TransportStream.LegacyTransportStream({ transport: { log(info, cb) { cb(); }, on() {} }});

(That said, doing something that causes the above issue would pretty unusual for people using this module directly. But Winston does it, which causes problems for downstream Winston users.)

Anyway, this resolves the issue in a non-breaking way by moving the definition of TransportStream to its own module and having LegacyTransportStream depend on that. Then the index.js module just imports both and adds LegacyTransportStream as a property on TransportStream. In an ideal world, I think these would just be two entirely separate properties on a plain old exports object, but that would be a breaking change that probably isn't worthwhile.