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

Inconsistency regarding configure signature of AppenderModule with typescript #1292

Closed
ofekisr opened this issue Jul 20, 2022 · 3 comments · Fixed by #1304
Closed

Inconsistency regarding configure signature of AppenderModule with typescript #1292

ofekisr opened this issue Jul 20, 2022 · 3 comments · Fixed by #1304

Comments

@ofekisr
Copy link

ofekisr commented Jul 20, 2022

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

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.

return appenderModule.configure(
adapters.modifyConfig(appenderConfig),
layouts,
(appender) => getAppender(appender, config),
levels
);

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

function configure() {

function configure(config, layouts) {

function configure(config, layouts, findAppender) {

function configure(config, layouts, findAppender, levels) {

@ofekisr
Copy link
Author

ofekisr commented Jul 26, 2022

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

@ofekisr Pull requests are always welcomed!

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 a pull request may close this issue.

2 participants