Skip to content

Revert remove type definitions #162

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

Closed
wants to merge 1 commit into from
Closed

Conversation

joyarzun
Copy link

Relate to #161

This revert deleted lines of type definition related to Winston logger

@joyarzun joyarzun force-pushed the master branch 2 times, most recently from 5f2fd43 to e2ae50a Compare November 27, 2019 19:27
Signed-off-by: Jonnatan Oyarzún <jonsxaero@gmail.com>
@joyarzun
Copy link
Author

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

@okkez
Copy link
Contributor

okkez commented Nov 28, 2019

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
Copy link
Author

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
Copy link
Contributor

okkez commented Nov 28, 2019

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

I will test this PR and release new version ASAP.

@okkez okkez self-assigned this Nov 29, 2019
@okkez
Copy link
Contributor

okkez commented Dec 2, 2019

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
Copy link
Contributor

okkez commented Dec 2, 2019

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;
 

okkez added a commit that referenced this pull request Dec 14, 2019
Fix #161
Close #162

Signed-off-by: okkez <okkez000@gmail.com>
@okkez okkez closed this in #163 Dec 14, 2019
okkez added a commit that referenced this pull request Dec 14, 2019
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