log4js-node icon indicating copy to clipboard operation
log4js-node copied to clipboard

Custom appender support for ESM project

Open jmeis opened this issue 3 years ago • 9 comments

Hi, I am migrating a project from CommonJS to pure ESM. Is it possible to load a custom appender at this time using pure ESM?

jmeis avatar Jan 11 '22 17:01 jmeis

ESM should be able to load CommonJS but not vice versa as far as I recall.

lamweili avatar Jan 27 '22 17:01 lamweili

appender "/home/nicojs/github/stryker-js/packages/core/dist/src/logging/multi-appender.js" could not be loaded (error was: Error [ERR_REQUIRE_ESM]: require() of ES Module /home/nicojs/github/stryker-js/packages/core/dist/src/logging/multi-appender.js from /home/nicojs/github/stryker-js/packages/core/node_modules/log4js/lib/appenders/index.js not supported.
Instead change the require of multi-appender.js in /home/nicojs/github/stryker-js/packages/core/node_modules/log4js/lib/appenders/index.js to a dynamic import() which is available in all CommonJS modules.)

So the require in appenders/index.js should be changed to an import.

Probably a major change.

nicojs avatar Feb 08 '22 22:02 nicojs

@nicojs, is line 27 the correct line to change? https://github.com/log4js-node/log4js-node/blob/2f29d0a0f5c2fcb8c5f58d9d6aeec1712ee104b7/lib/appenders/index.js#L24-L32

Would you be kind enough to do a PR? 😊

lamweili avatar Feb 09 '22 13:02 lamweili

I tried to change the codes in appenders/index.js, but to no avail.

- const tryLoading = (modulePath, config) => { 
+ const tryLoading = async (modulePath, config) => { 
=   debug('Loading module from ', modulePath); 
=   try { 
-     return require(modulePath); // eslint-disable-line
+     return (await import(modulePath)).default;
=   } catch (e) { 
=     // if the module was found, and we still got an error, then raise it 
=     configuration.throwExceptionIf( 
=       config, 
-       e.code !== 'MODULE_NOT_FOUND', 
+       e.code !== 'ERR_MODULE_NOT_FOUND', 
- const createAppender = (name, config) => {
+ const createAppender = async (name, config) => {
=   const appenderConfig = config.appenders[name];
=   const appenderModule = appenderConfig.type.configure
-     ? appenderConfig.type : loadAppenderModule(appenderConfig.type, config);
+     ? appenderConfig.type : await loadAppenderModule(appenderConfig.type, config);

Would need any help anyone can render.

lamweili avatar Feb 09 '22 17:02 lamweili

Yes, this is what I meant by major change. Simply making some functions async won't work.

In the end, I think the log4js.configure function should become an async function. Appenders should then be created one by one I think.

nicojs avatar Feb 09 '22 18:02 nicojs

You might want to go all-the-way and migrate this library itself to esm.

nicojs avatar Feb 09 '22 19:02 nicojs

Related to #1156.

lamweili avatar Feb 10 '22 04:02 lamweili

@jmeis, you can actually use it right now.

The CustomAppender type allows you to use any object with a configure function.

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

export interface AppenderModule {
  configure: (config: Config, layouts: LayoutsParam) => AppenderFunction;
}

So, if you import your custom Appender ahead of time, you can use it that way.

import appender from './customAppender.js';
log4js.configure({
  appenders: {
    custom: { type: appender, customProperty1: false, customProperty2: (a,b)=>a+b }
  },
  categories: { default: { appenders: ['custom'], level: 'info' } }
});

I'd actually recommend that for the next major version, log4js adopts this as the main way to use an appender instead of the string type property to make it more clear what you are importing to use each. I think it would also assist with making strongly typed appender definitions definitions much easier, though I'd have to play around with that to really know for sure.

ZachHaber avatar Jun 14 '22 14:06 ZachHaber

Thanks, @ZachHaber!

Alternatively, if your project is in ESM and your custom appender can be in CommonJS, rename your custom appender to have the .cjs extension - https://github.com/log4js-node/log4js-node/issues/1277#issuecomment-1173149974.

lamweili avatar Jul 03 '22 16:07 lamweili