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

Custom appender support for ESM project #1122

Open
jmeis opened this issue Jan 11, 2022 · 9 comments
Open

Custom appender support for ESM project #1122

jmeis opened this issue Jan 11, 2022 · 9 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@jmeis
Copy link

jmeis commented Jan 11, 2022

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?

@lamweili
Copy link
Contributor

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

@nicojs
Copy link
Contributor

nicojs commented Feb 8, 2022

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.

@lamweili
Copy link
Contributor

lamweili commented Feb 9, 2022

@nicojs, is line 27 the correct line to change?

const tryLoading = (modulePath, config) => {
debug('Loading module from ', modulePath);
try {
return require(modulePath); // eslint-disable-line
} catch (e) {
// if the module was found, and we still got an error, then raise it
configuration.throwExceptionIf(
config,
e.code !== 'MODULE_NOT_FOUND',

Would you be kind enough to do a PR? 😊

@lamweili lamweili added this to the 6.4.2 milestone Feb 9, 2022
@lamweili lamweili added enhancement New feature or request help wanted Extra attention is needed labels Feb 9, 2022
@lamweili
Copy link
Contributor

lamweili commented Feb 9, 2022

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.

@nicojs
Copy link
Contributor

nicojs commented Feb 9, 2022

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

nicojs commented Feb 9, 2022

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

@lamweili
Copy link
Contributor

Related to #1156.

@lamweili lamweili removed this from the 6.4.2 milestone Feb 10, 2022
@ZachHaber
Copy link
Contributor

@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.

@lamweili
Copy link
Contributor

lamweili commented Jul 3, 2022

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 - #1277 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants