-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat: Unify logging behavior across environments #4416
Conversation
…I when copying the config file.
Is there ever a situation where we'd native native logs but not webview logs? Would that be an option worth exploring? |
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.
Added a few minor comments.
I personally don't like the "production" name. There is already a lot of confusion between the web production build and the native production build. Having a "production" value looks like it's the value you should use for the production build, so can lead to users enabling logs in production builds while thinking they are disabling it.
I'm open to ideas. "release", perhaps? I was trying to keep the values short but we could use longer, more descriptive strings like "allowInProduction" or "disabledForRelease" or something. But those could get awkward. |
@thomasvidas I can't think of one. We could offer it but, logically, then we should also offer the inverse, i.e. web logs but not native logs. But that feels like a lot of complication for a relatively simple feature. You'll need to parse a bunch of noise to find the info you need no matter what. I think the biggest concern is leaking sensitive information by logging in release builds without realizing it (which can happen now). |
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.
LGTM.
bridge.ts moved over to native-bridge.js in my PR, but I think everything else should merge in cleanly
# Conflicts: # android/capacitor/src/main/java/com/getcapacitor/Bridge.java # core/src/bridge.ts
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.
two minor things
This branch updates the rules for when/how statements should be logged at runtime to reconcile differences between JS and native implementations. Previously, in the default case of
hideLogs
in the config file being either false or missing, the native code would log statements in production builds while the JS code would not. The previous boolean flag was insufficient for describing the state(s).loggingBehavior
is a new config option with possible values ofnone
,debug
, andproduction
. Both JS and native code follow the same rules:none
, no log statements are generated.debug
(the default), log statements are generated in debug builds but not production builds.production
, log statements are generated in all builds.hideLogs
is now deprecated in the config file with a warning in the CLI. IfloggingBehavior
is absent buthideLogs
exists,false
is treated asdebug
andtrue
is treated asnone
.isLoggingEnabled
flag to avoid the conflation between debug and logging states.Companion documentation change: ionic-team/capacitor-site#258
Closes #3723