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

fix: Logger has not export in entry file #1276

Merged
merged 1 commit into from Jul 6, 2022

Conversation

taozi0818
Copy link
Contributor

@taozi0818 taozi0818 commented Jun 24, 2022

Fix #1275

Copy link
Contributor

@lamweili lamweili left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

History in 2.0.0

It was changed from interface to class back in a1e40c2 which was released as 2.9.0.
It was probably due to the exposure of Logger as a global variable on configure().

Logger = loggerModule.Logger;

log4js-node/lib/logger.js

Lines 134 to 137 in a1e40c2

return {
LoggingEvent: LoggingEvent,
Logger: Logger
};

Refactored in 3.0.0

The global variable Logger has since been removed in 8084e80 which was released as 3.0.0.
But there was no change to the typings log4js.d.ts.


Thus, would a single line change from class to interface in log4js.d.ts (a reversal of a1e40c2) be better?

export class Logger {

-        export class Logger { 
+        export interface Logger { 

Because Logger should not be used standalone nor exposed. It won't work if log4js is not configured (which is also handled silently by getLogger()).

It is wrapped around and should only be retrieved from getLogger().

log4js-node/lib/log4js.js

Lines 144 to 154 in 3582a00

function getLogger(category) {
if (!enabled) {
configure(
process.env.LOG4JS_CONFIG || {
appenders: { out: { type: 'stdout' } },
categories: { default: { appenders: ['out'], level: 'OFF' } },
}
);
}
return new Logger(category || 'default');
}

@lamweili lamweili added this to the 6.5.3 milestone Jun 27, 2022
@taozi0818
Copy link
Contributor Author

OK. Both methods are available. The key point is keeping them consistent.

@lamweili
Copy link
Contributor

lamweili commented Jul 6, 2022

@taozi0818 Thanks! I rebased.

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.

Class Logger has not been export from entry file
2 participants