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

Remove nonexistent Logger methods from types #2339

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

JavaScriptBach
Copy link
Contributor

Remove nonexistent Logger methods from types. Fixes #2280

Testing

Welcome to Node.js v18.16.1.
Type ".help" for more information.
> const winston = require("winston")
undefined
> const logger = winston.createLogger({})
undefined
> logger.warning
undefined
> logger.emerg
undefined
> logger.alert
undefined
> logger.crit
undefined
> logger.notice
undefined

@wbt
Copy link
Contributor

wbt commented Aug 25, 2023

This PR removes some lines that were most recently moved in #1807 and moved back in #1820 due to #1817.
They were originally added in #1190, a broad PR adding TypeScript definitions, with notes warning about insufficient testing.
The proposed change looks good to me but I'll leave it open a bit for possible addition of regression tests and any other feedback, especially as this looks like it will cause some things to fail at transpile-time that previously would pass that and generally fail at runtime instead.

@JavaScriptBach
Copy link
Contributor Author

With all due respect, I think you're overthinking this change. This PR only changes types, not runtime behavior, so there is nothing to regression test. And the whole point of types is catch broken code at compile time, instead of at runtime. The fact that the types currently 1) allow broken code to type check and 2) imply the existence of methods that don't exist is a bug that IMO should be fixed ASAP.

@provAlyssaKrueger
Copy link

@wbt any chance we can get the proposed fix merged?
The existing types are misleading and pose a risk for causing runtime errors.

At the very least, you should consider making the methods optional so a caller using TypeScript strict null checks is aware.

@wbt wbt merged commit c3c3911 into winstonjs:master Oct 18, 2023
@codazzo
Copy link

codazzo commented Jan 8, 2024

Thanks for the fix @JavaScriptBach. @wbt would you consider releasing a new version of the package so users of the package can get it?

@derekdowling
Copy link

derekdowling commented Feb 28, 2024

Hello @wbt can we please get a version release with this change? The incorrect typings just broke us in production.

@codazzo
Copy link

codazzo commented Feb 29, 2024

Perhaps @DABH could help with a new release?

@DABH
Copy link
Contributor

DABH commented Mar 4, 2024

I've just released winston v3.12.0. I hope this helps!

@wbt
Copy link
Contributor

wbt commented Apr 2, 2024

#2424 demonstrates the validity of being quite hesitant to merge!

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.

[Bug]: type hints say there's a logger.warning() but it does not exist
6 participants