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

6.4.0 introduced a breaking type change through #1117 #1178

Closed
simhnna opened this issue Jan 27, 2022 · 11 comments · Fixed by #1117
Closed

6.4.0 introduced a breaking type change through #1117 #1178

simhnna opened this issue Jan 27, 2022 · 11 comments · Fixed by #1117

Comments

@simhnna
Copy link

simhnna commented Jan 27, 2022

Not sure if you're following semver or not, but this change broke our code.

Any chance we can maybe go to _log(level: Level | string, data: any) and internally converting strings to Level?

@lamweili
Copy link
Contributor

lamweili commented Jan 27, 2022

In any case, you should be using log. Would using log suffice?
It does additional checks for isLevelEnabled before proceeding further to _log (non-public).

log(level: Level | string, ...args: any[]): void;

log(level, ...args) {
let logLevel = levels.getLevel(level);
if (!logLevel) {
this._log(levels.WARN, 'log4js:logger.log: invalid value for log-level as first parameter given: ', level);
logLevel = levels.INFO;
}
if (this.isLevelEnabled(logLevel)) {
this._log(logLevel, args);
}
}

@simhnna
Copy link
Author

simhnna commented Feb 18, 2022

for some historic reason our logger implements the Logger class and doesn't extend it. I've pinned our dependencies to an older version for now. I'll see if I can refactor our code.

If you want you can close the issue

@lamweili
Copy link
Contributor

The Logger class has both log() - public and _log() - internal.
You probably have to refactor your logger to use .log() instead of ._log().

I'll leave this issue open for now and check back with you again after 2 weeks.

@lamweili lamweili changed the title 6.4.0 introduced a breaking type change through !1117 6.4.0 introduced a breaking type change through #1117 Feb 20, 2022
@lamweili lamweili linked a pull request Feb 20, 2022 that will close this issue
@lamweili
Copy link
Contributor

lamweili commented Mar 8, 2022

._log() is an internal API. Please migrate over to .log().
There are differences between .log() and ._log() as shown in the example below that might help you.

const log4js = require("log4js");
log4js.configure({
  appenders: {
    normal: { type: 'stdout' }
  },
  categories: {
    default: { appenders: ['normal'], level: 'info' }
  }
});
const logger = log4js.getLogger();

logger.info("hello"); // String
// [2022-03-09T01:13:38.129] [INFO] default - hello

logger.log("info", "hello"); // String, String
// [2022-03-09T01:13:38.135] [INFO] default - hello

logger._log("info", "hello"); // String, String
// [2022-03-09T01:13:38.139] [info] default - h e l l o

const Level = require("log4js/lib/levels");
logger._log(Level.INFO, ["hello"]); // Level, String[]
// [2022-03-09T01:13:38.149] [INFO] default - hello

image

@lamweili
Copy link
Contributor

@simhnna, did you managed to refactor?

@simhnna
Copy link
Author

simhnna commented Oct 5, 2022

yeah. I refactored this. But then again 126e286 was also a change that removed an exported type.

I'm currently pinning log4js due to such changes

@lamweili
Copy link
Contributor

lamweili commented Oct 5, 2022

yeah. I refactored this. But then again 126e286 was also a change that removed an exported type.

I'm currently pinning log4js due to such changes

@simhnna, which is the removed exported type in 126e286?
I see BaseLayout being renamed to BasicLayout (which shouldn't affect anything) and changes to comments. 🤔

@simhnna
Copy link
Author

simhnna commented Oct 11, 2022

I mean the missing BaseLayout. Renaming something is like removing it. I have log4js defined as a peer dependency (^6.0.0) and such changes break our builds if anyone pulls in the latest version

@lamweili
Copy link
Contributor

lamweili commented Oct 11, 2022

@simhnna, I see. Perhaps you should also rename BaseLayout to BasicLayout as part of the refactoring so that you can update to have bug fixes.

Of course, you can also stick with the pinned version if it solves the problem.

@lamweili
Copy link
Contributor

If you want you can close the issue

@simhnna, can we close the issue?

@lamweili
Copy link
Contributor

Closed due to extended period of inactivity.

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