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

TS definition: incorrect signatures with callback argument #1922

Closed
evil-shrike opened this issue May 21, 2021 · 6 comments · Fixed by #2513
Closed

TS definition: incorrect signatures with callback argument #1922

evil-shrike opened this issue May 21, 2021 · 6 comments · Fixed by #2513

Comments

@evil-shrike
Copy link

There're bunch of method signatures that contain callback:

   interface LogMethod {
    (level: string, message: string, callback: LogCallback): Logger;
    (level: string, message: string, meta: any, callback: LogCallback): Logger;
    (level: string, message: string, ...meta: any[]): Logger;
    (entry: LogEntry): Logger;
    (level: string, message: any): Logger;
  }

  interface LeveledLogMethod {
    (message: string, callback: LogCallback): Logger;
    (message: string, meta: any, callback: LogCallback): Logger;
    (message: string, ...meta: any[]): Logger;
    (message: any): Logger;
    (infoObject: object): Logger;
  }

But actually looking at js code it doesn't seem that Winston supports any callbacks for log methods at all.

@tokidoki11
Copy link
Contributor

LogCallback is indeed not supported anymore as per version 3 and can cause confusion

https://github.com/winstonjs/winston/blob/HEAD/UPGRADE-3.0.md#winstonlogger
winston.Logger.log and level-specific methods (.info, .error, etc) no longer accepts a callback.

@DABH
Copy link
Contributor

DABH commented Oct 3, 2024

Ah, if this is the case, PR is welcomed here...

@tokidoki11
Copy link
Contributor

@DABH FYI i just made a PR for that

@DABH
Copy link
Contributor

DABH commented Oct 4, 2024

Thanks, I'll take a look... I need to think a little carefully here, because this is a breaking change, but it seems like this should not have been here in the first place... so not sure if this needs to be a major or a minor version release of winston. Feels a little funny to do a major release over some TS changes but I don't want to throw off anyone who might be using these definitions (although the thing the defs are referring to aren't supported...). @wbt any thoughts on how we semver this, if we merge it in? Open to other's opinions as well

@tokidoki11
Copy link
Contributor

@DABH
IMHO

minor version should be okay since the functionality itself is not changing. meanwhile, as you said earlier, that the ones who are using this definitions might get shocked on their build.
I think that is good in a sense that they get notified the callback is not run at all.

@DABH
Copy link
Contributor

DABH commented Oct 5, 2024

Yeah I guess I'll do that... and let you handle any complaints from users that come in ;)

@DABH DABH closed this as completed in #2513 Oct 5, 2024
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.

3 participants