Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve circular dependency issues downstream in Winston #207

Conversation

Mr0grog
Copy link
Contributor

@Mr0grog Mr0grog commented Jan 30, 2024

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.

There is a circular dependency between the `TransportStream` (formerly in `index.js`) and `LegacyTransportStream` (in `legacy.js`). This causes some funky problems when you import the legacy module and later import the main module, as seen in winstonjs/winston#1886. I've resolved the circular issue here by definiting the main implementation of `TransportStream` in a separate module (which `LegacyTransportStream` now depends on) and then doing the work of decorating it with a `.LegacyTransportStream` property in `index.js` (and that's pretty much all the index module does).
Copy link
Contributor

@DABH DABH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with this! Thanks for doing things in a non-breaking way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants