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(android): move Config to be per-instance rather than a singleton #3055

Merged
merged 3 commits into from
Jun 8, 2020

Conversation

jcesarmobile
Copy link
Member

@jcesarmobile jcesarmobile commented Jun 4, 2020

equivalent of e964068 for Android

Used a JSONObject instead of a JSON formatted string (what iOS does) because JSONObject can be created from a JSON formatted string already, and can also put values in a simpler way than a JSON formatted string.

thoughts @carlpoole @dwieeb?

@imhoffd
Copy link
Contributor

imhoffd commented Jun 4, 2020

I don't know either, other than passing in config for every log method call 🤮

@carlpoole
Copy link
Member

carlpoole commented Jun 4, 2020

I think it would be messy if we had a logger instance also per Config instance so I'm thinking have shouldLog as a static global variable on Logger that can be set by doing something like

Logger.enabled(true)

and making it an all or nothing thing. I don't like the idea of tying the logger to the config when there is a possibility of having multiple configs. Or maybe just have it be enabled globally if at least one config asked for it.

@jcesarmobile
Copy link
Member Author

I've passed the config to logger on bridge init and now it works.

I've noticed that there is a "Starting BridgeActivity" log that is always displayed since it's logged before the config is set, but was also happening with previous implementation.

@carlpoole
Copy link
Member

Looks good!

@jcesarmobile jcesarmobile marked this pull request as ready for review June 5, 2020 16:52
@jcesarmobile jcesarmobile merged commit b4815a5 into master Jun 8, 2020
@jcesarmobile jcesarmobile deleted the config-no-singleton branch June 8, 2020 09:04
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.

None yet

3 participants