-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Added condition wrapper for only R.N vs supported #4447
Added condition wrapper for only R.N vs supported #4447
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Rolando-Barbella,
thanks a lot for this request!
It's the right direction, but I was aiming for an if
to be implemented under the attach()
method (i.e. making the logger a no-op). I will definitely accept and merge that. In any case, why is the condition minor ≥ 71000
and not ≥71
?
Hi @d4vidi thanks for the feedback, I just updated the branch with the changes, hopefully it makes more sense now |
@Rolando-Barbella you don't need the isLoggerEnabled toggle. The entire class is a listener which unless registered, it does nothing. |
@d4vidi sorry, I have little experience with Kotlin, if the logic inside the attach method is not needed, what do you mean by an if to be implemented under the attach() method (i.e. making the logger a no-op) ? Do you consider something like this?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rolando-Barbella check out my suggestion. This is everything you need.
if (ReactNativeInfo.rnVersion().minor >= 71) { | ||
isLoggerEnabled = true | ||
} else { | ||
ReactMarker.addListener(this) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (ReactNativeInfo.rnVersion().minor >= 71) { | |
isLoggerEnabled = true | |
} else { | |
ReactMarker.addListener(this) | |
} | |
if (ReactNativeInfo.rnVersion().minor >= 71) { | |
ReactMarker.addListener(this) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@d4vidi much simpler approach! I updated the branch, thank you
Description
Following issue #4445 I added a condition that limits react logger for not supported versions as @d4vidi recommend it