fluent / fluent-logger-node

A structured logger for Fluentd (Node.js)
Apache License 2.0
259 stars 83 forks source link

Revert remove type definitions #162

Closed joyarzun closed 4 years ago

joyarzun commented 4 years ago

Relate to #161

This revert deleted lines of type definition related to Winston logger

joyarzun commented 4 years ago

Hi @okkez. There is needed something else to merge?

okkez commented 4 years ago

Winston support is optional. Because some users don't want to install winston npm package and they want to use this library with TypeScript.

Therefore we cannot merge this PR.

joyarzun commented 4 years ago

Winston support is optional. Because some users don't want to install winston npm package and they want to use this library with TypeScript.

Therefore we cannot merge this PR.

But it isn't necessary to install Winston to have the support type. Plus the Winston support is part of the doc so dropping types can be ugly to typescript devs. Anyway if this PR can't be merged, what can be the workaround to Property 'support' does not exist on type error?

okkez commented 4 years ago

Ah... I got it. Sorry for my misunderstanding.

I will test this PR and release new version ASAP.

okkez commented 4 years ago

I've tested this PR with a simple script. The compiler reports the following error.

node_modules/fluent-logger/lib/index.d.ts:80:43 - error TS2304: Cannot find name 'FluentTransport'.

80     winstonTransport: () => Constructable<FluentTransport, Options>
                                             ~~~~~~~~~~~~~~~
import winston from "winston";
import Fluent from "fluent-logger";
const config = {
  host: 'localhost',
  port: 24224,
  timeout: 3.0,
  requireAckResponse: true // Add this option to wait response from Fluentd certainly
};
// const fluentTransport = require('fluent-logger').support.winstonTransport();
const fluentTransport = Fluent.support.winstonTransport();
const logger = winston.createLogger({
    transports: [new fluentTransport('mytag', config), new (winston.transports.Console)()]
});

logger.on('logging', (transport, _level, _message, meta) => {
  if (meta.end && transport.sender && transport.sender.end) {
    transport.sender.end();
  }
});

logger.log('info', 'this log record is sent to fluent daemon');
logger.info('this log record is sent to fluent daemon');
logger.info('end of log message', { end: true });

In this case, we cannot resolve this error without importing winston possibly. We can split out FluentTransport as an individual npm package like fluent-appender-log4js. How do you think about this? Or any idea?

okkez commented 4 years ago

Or, how about the following?

diff --git a/lib/index.d.ts b/lib/index.d.ts
index c5a8943..040fe69 100644
--- a/lib/index.d.ts
+++ b/lib/index.d.ts
@@ -76,9 +76,9 @@ declare namespace fluentLogger {
     static fromTimestamp(t: number): InnerEventTime;
   }

-  interface Constructable<T, U> {
-    new(tag: string, options: U) : T;
-  }
+  let support: {
+    winstonTransport: any
+  };

   let EventTime: InnerEventTime;