hippware / rn-chat

MIT License
5 stars 0 forks source link

DO NOT MERGE: no need to inject global `console` #4884

Closed southerneer closed 4 years ago

southerneer commented 4 years ago

Based on my questions on #4873

@bengtan if we modify the global console object then there's no need to inject it into Transport, Wocky, etc., right? Maybe I'm missing something.

southerneer commented 4 years ago

And on second thought, monkey patching console is a bad idea precisely because it's available globally. NPM modules shouldn't use console for logging, but the mere fact that it's possible makes it bad.

bengtan commented 4 years ago

Happy to discuss.

When I wrote up the ticket #4866, I had already gone through a few iterations of logic before even starting PR #4873. Happy to explain the reasoning.

Also, please read this in conjunction with #4866.

if we modify the global console object then there's no need to inject it into Transport, Wocky, etc., right? Maybe I'm missing something.

True. Since console has been modified/augmented, our code can also directly call console to do logging/bugsnag/logging-to-RNBGL-log

BUT

There are some arguments that app code should not (in the general case) call console.log. I don't have them off the top of my head but you can find them on the Net.

I'm 50/50 about it. I can work with either calling console or not calling console.

But historically, our policy is that we don't call console.

I care more about consistency so I didn't want to call this policy into question.

So, yes, our code can directly call console but I didn't want to make widespread changes (including policy changes).


Hmmm ... just reading through what I wrote above and thinking a bit more ...

There is a reason for us not to be calling console.

I don't really like the idea of 'monkey patching' console and having our code call console.bugsnagNotify. Hence, putting a (albeit very thin) wrapper/abstraction around console and treating it as a custom logger.


And on second thought, monkey patching console is a bad idea precisely because it's available globally. NPM modules shouldn't use console for logging, but the mere fact that it's possible makes it bad.

Yes, I agree that monkey patching console is a bad idea.

But I needed to put code in @absinthe/socket to post bugsnags. (And although you don't see it in my PRs, I was also logging to the RNBGL log in @absinthe/socket whilst debugging ticket 4004.)

@absinthe/socket isn't even a direct dependency of our code. It's a dependency of a dependency. I wasn't going to inject a logger object into that.

I needed to use some sort of global object, and console seemed to be a least-evil choice.


And one complicating factor is that ... whatever changes we make, the wocky-client code needs to be able to execute in both RN and nodejs environments.

southerneer commented 4 years ago

But I needed to put code in @absinthe/socket to post bugsnags. (And although you don't see it in my PRs, I was also logging to the RNBGL log in @absinthe/socket whilst debugging ticket 4004.)

OK, I think this is the crux of the the problem. And wow, I didn't even see https://github.com/hippware/rn-chat/pull/4874 until today. I think there's a cleaner way of handling all of this. I'll work on it today.

southerneer commented 4 years ago

closing in favor of https://github.com/hippware/rn-chat/pull/4887