search

hippware / rn-chat

MIT License
5 stars 0 forks source link

4866 logging improvements #4873

Closed bengtan closed 4 years ago

bengtan commented 4 years ago

Please see #4866 for rationale and discussion.

No need to rush merging this PR.

There might be parts which could be controversial.

Happy to discuss and answer any questions or concerns.

aksonov commented 4 years ago

LGTM

southerneer commented 4 years ago

@aksonov given Beng's comments in the OP I would have liked the opportunity to review this before merging.

If that was your intention @bengtan you can explicitly specify multiple reviewers in the Reviewers section to the right (probably you already knew that...apologies if so).

image

bengtan commented 4 years ago

Wouldn't it be cleaner to just import a log method? I don't see any benefit to making logger a class property.

Same issue here...I don't see the value add of injecting into env

Reasons:

  1. It allows the injection of different loggers based on the environment.

When running in the app, Wocky and Transport are injected with a logger that can post to RNBGL and bugsnag.

When running in nodejs, Wocky and Transport are injected with a logger that routes RNBGL logging and bugsnag calls to console.

  1. It allows wocky-client/src/logger.ts to be removed.
southerneer commented 4 years ago

It allows the injection of different loggers based on the environment.

Right, I see the need to differentiate functionality based on environment, but modifyConsoleAndGetLogger and augmentConsole take care of that. I don't see the further benefit of injecting from there. It's probably easier to just ask my question with a simple PR.

southerneer commented 4 years ago

I believe we need to revert/rewrite this PR because of this.

bengtan commented 4 years ago

I don't see the further benefit of injecting from there. It's probably easier to just ask my question with a simple PR.

I believe we need to revert/rewrite this PR because of this.

Going to reply at PR #4884.