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

feat(ios): add useLog config #1431 #1640

Closed
wants to merge 2 commits into from
Closed

Conversation

stewones
Copy link
Contributor

@stewones stewones commented Jun 6, 2019

@stewones
Copy link
Contributor Author

stewones commented Jun 6, 2019

this PR provides a way to disable native logs #1431. you can control it on your capacitor.config.json

{
  "useLog": false
}

Update

  • changed config option to production, so we can use that for more than one purpose (just like angular team do)
{
  "production": true
}
  • abstraction of log things into its own class

usage

CAPLog.add(message: "Hey! I'm a log")

@stewones
Copy link
Contributor Author

stewones commented Jun 6, 2019

I can add android version, just let me know if the api fits good.

@jcesarmobile
Copy link
Member

I think this print should be a drop in replacement for regular print, so should look like

 public static func print(_ items: Any..., separator: String = " ", terminator: String = "\n") {
        if (self.useLog() as? Bool != false) {
            Swift.print(items, separator, terminator)
        }
    }

No need to have the @objc as print is Swift only (same for the other methods and class)

Not sure if we need this new CAPLog class, or if we can just put it inside the CAPBridge class, as it has a modulePrint function already, or move that one to the new CAPLog class.

Another thing, CAPConfig is no longer a singleton, so code should be changed.
First a static config should be declared
public static let config = CAPConfig()

And change the usage to
let isProduction = config.getValue("production") as? Bool

But I not sure about the production name of the preference, in my experience, people tend to think they are building a production app just by setting that value to true, so I would stick with showLogs, or hideLogs, or useLogs

Or maybe just don't make it configurable and hide all the logs when a release build is done with the DEBUG macro

#if DEBUG
  print(...)
#endif

@mlynch thoughts?

@stewones
Copy link
Contributor Author

drop in replacement looks awesome. thanks for the ideas.

from my work experience macros won't work. what if I wanted to check capacitor logs with my app in production mode.

@jcesarmobile
Copy link
Member

As Max said we shouldn't show them by default, I suppose he means in production.
So it should check both isDevMode method from CAPBridge and the configuration option in the capacitor.config.json to always show them in dev mode, but in production only if enabled with the preference.

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.

2 participants