log4js-node / log4js-node

A port of log4js to node.js
https://log4js-node.github.io/log4js-node/
Other
5.8k stars 773 forks source link

Inconsistency regarding configure signature of AppenderModule with typescript #1292

Closed ofekisr closed 2 years ago

ofekisr commented 2 years ago

There is some inconsistency between the documentation and typescript types regarding the configure signature of AppenderModule and there is no findAppender declaration in log4js.d.ts

What do you think?

please review the three states below:

from: appenders Advanced configuration doc - configure defined with four arguments

const myAppenderModule = {
  configure: (config, layouts, findAppender, levels) => {
    /* ...your appender config... */
  },
};
log4js.configure({
  appenders: { custom: { type: myAppenderModule } },
  categories: { default: { appenders: ["custom"], level: "debug" } },
});

from: writing appenders doc - configure defined with only two arguments

// This is the function that generates an appender function
function stdoutAppender(layout, timezoneOffset) {
  // This is the appender function itself
  return (loggingEvent) => {
    process.stdout.write(`${layout(loggingEvent, timezoneOffset)}\n`);
  };
}

// stdout configure doesn't need to use findAppender, or levels
function configure(config, layouts) {
  // the default layout for the appender
  let layout = layouts.colouredLayout;
  // check if there is another layout specified
  if (config.layout) {
    // load the layout
    layout = layouts.layout(config.layout.type, config.layout);
  }
  //create a new appender instance
  return stdoutAppender(layout, config.timezoneOffset);
}

//export the only function needed
exports.configure = configure;

from: https://github.com/log4js-node/log4js-node/blob/master/types/log4js.d.ts - configure defined without findAppender there is no declaration of findAppender - I can't defined my own Appender as middleware Appender - I can't reach the next appenders

export interface CustomAppender {
  type: string | AppenderModule;
  [key: string]: any;
}

export interface AppenderModule {
  configure: (config: Config, layouts: LayoutsParam) => AppenderFunction;
}
lamweili commented 2 years ago

Thanks, I have corrected the typings in PR #1304.


On initialisation, the index.js will always pass the 4 parameters to configure the specific appender. https://github.com/log4js-node/log4js-node/blob/0c37f4063e5bada95803087722a57cf9f2034a50/lib/appenders/index.js#L117-L122

But it is up to individual appenders whether to use and take in the parameters.

https://github.com/log4js-node/log4js-node/blob/0c37f4063e5bada95803087722a57cf9f2034a50/lib/appenders/recording.js#L5

https://github.com/log4js-node/log4js-node/blob/0c37f4063e5bada95803087722a57cf9f2034a50/lib/appenders/file.js#L135

https://github.com/log4js-node/log4js-node/blob/0c37f4063e5bada95803087722a57cf9f2034a50/lib/appenders/categoryFilter.js#L14

https://github.com/log4js-node/log4js-node/blob/0c37f4063e5bada95803087722a57cf9f2034a50/lib/appenders/logLevelFilter.js#L15

ofekisr commented 2 years ago

@lamweili thanks!! you should fix the documentation as well to be more precise, I understand how to build a custom appnder via the src code and not only from the documentation You can contact me for more details ofek1israel@gmail.com

lamweili commented 2 years ago

@ofekisr Pull requests are always welcomed!