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

Logger names are empty in JS/IR when declaring loggers via KotlinLogging.logger {} #315

Open
YarnSphere opened this issue May 23, 2023 · 5 comments

Comments

@YarnSphere
Copy link
Contributor

This behaviour might have changed from Kotlin JS/Legacy, but Kotlin JS/IR doesn't seem to keep information on fully qualified names at runtime (mentioned in the documentation here, and there are also a few related issues open in YouTrack: [1], [2]).

Due to this, the current implementation of KotlinLogging.logger {} creates a logger with an empty string for name in JS/IR.

I propose that kotlin-logging tries to use the class' simpleName for name of the logger in JS (at least until there is a way to obtain a qualified name in JS, if they ever plan to support it).

I'm unsure whether this would be possible with the current implementation where the string is obtained from a stack trace (would have to give it a try), but something that would surely work would be an alternative way of declaring a logger using a function with a reified type: KotlinLogging.logger<RelevantClass>().

The implementation could use the qualifiedName of RelevantClass on all platforms except JS, where the simpleName would be used instead (this function would also feel a tad less hacky/brittle than the current one where the logger name is obtained from a stack trace 😛).

I could probably have a go at implementing this at some point, but let me know if you agree with the proposal and in which direction you'd rather go: attempt to make KotlinLoggin.logger {} work properly in JS/IR (unsure if possible), provide a new KotlinLogging.logger<RelevantClass>() implementation, or even both.

@oshai
Copy link
Owner

oshai commented May 24, 2023

We already encountered a similar issue and the solution was to create another impl for js. You can see it here: https://github.com/oshai/kotlin-logging/blob/master/src/jsMain/kotlin/io/github/oshai/KotlinLogging.kt

From previous testing JS worked differently for browser / node etc' so it was difficult to have one impl that fits all.

See detailed discussion at: #279

Let me know what you think.

@YarnSphere
Copy link
Contributor Author

The above implementation doesn't really help me, I'm defining my logger in common multiplatform code, not directly in the JS code. Besides, I wouldn't want the logger to be an instance variable.

I hadn't considered the node vs browser scenario. When I get a bit of time I'll try to understand the differences a bit better, but what do you think of my suggestion of a new fun KotlinLogging.logger<reified T>() implementation? I believe that T::class.simpleName should be well supported in either JS target (I'd have to test regardless). And T::class.qualifiedName well supported in all non-JS targets.

@recursive-rat4
Copy link

How can a top-level function (it does not have a relevant class) obtain a logger with this API?

@YarnSphere
Copy link
Contributor Author

YarnSphere commented May 24, 2023

It can't, I suppose. Not unless it specifies a class to defer their name to. It could at least be an alternative implementation for when a logger is associated with a class. Either way, it might be possible to fix the current problem in JS with KotlinLogging.logger {}, I'll have to give it a try at some point.

@severn-everett
Copy link
Contributor

Doesn't look like there's been any concrete movement on this from the Kotlin developers' side, and the issue KT-34534 seems to be a bit of a hot potato, at least right now.

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

No branches or pull requests

4 participants