-
Notifications
You must be signed in to change notification settings - Fork 774
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
Wrong TS types in 6.4.0? #1155
Comments
I think this was introduced in #1079 FWIW. |
Hi, our test cases passed as it didn't cover the typescript aspect. I concur with you that it might be something to do with the types and not the code. I don't use typescript, so I would need to rely on users like you to help me with it. If you have the solution ready, would you mind making a PR? I'll try to take a look at it when I have the time, and perhaps you can help me with running the tests before the next release? That'll be awesome. |
Fixes v6.4.0 regressions. There is an npm script `npm run typings` which actually failed before this commit. This seems to have been a combination of incorrect typings (a mandatory argument to `getLevel` that should be optional, the idea that for custom appenders `configure` should return an intermediate "generator" function before the appender function itself) as well as mistakes in the test file (which had an appender module which had one level of function nesting too few). Specifically, for the `configure` function on appenders, I believe there should be two levels of function: an outer function taking one-time configuration, and an inner function taking an individual log messages. The typings file wanted there to be three layers, and the test file wanted there to be one layer. Fixes log4js-node#1155.
Thanks, I filed a PR. It makes your existing TS compilation tests pass (they were failing before actually) with a combination of test and typing changes. |
Hi. My project doesn't directly use log4js but we do want to make sure that a
logger
argument we take is compatible withlog4js
objects, so our test suite sets up log4js and sends a message through it (via our ApolloServer object) and tests that it works.Renovate picked up the 6.4.0 release but this test file doesn't compile with the changes to your TypeScript types in v6.4.0.
I honestly don't understand log4js well enough to be sure, but it looks like the issue is specific to the types, not an actual change to your JS code.
Specifically, we call:
According to your types file, the
configure
function is a(config: Config, layouts: LayoutsParam) => AppenderGenerator
, and AppenderGenerator is(layout: LayoutFunction, timezoneOffset?: string) => AppenderFunction
, andAppenderFunction
is(loggingEvent: LoggingEvent) => void
.This is one more layer of "function" than the code above that previously worked for us: the outer
() =>
is the(config, layouts) =>
type, and the inner(loggingEvent) =>
function is an AppenderFunction, but we were not writing an AppenderGenerator in the middle.OK, I said. This seemed like an odd backwards-incompatibility to put out in a minor release, but sure. I added another
() =>
after the first() =>
above... and my test failed.I reverted that change and instead added an
as any
somewhere... and my test passed.So... I don't really know much about log4js. But it seems like what happened here is that v6.4.0 did not actually change your API as implemented in JS, but that the TypeScript changes introduced an intermediate function that isn't actually part of code.
Specifically it seems like maybe
AppenderGenerator
isn't a real thing and thatAppenderModule
should be declared asAm I missing something?
The text was updated successfully, but these errors were encountered: