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

[react-native] console.group not supported #147

Closed
cparjaszewski opened this issue Oct 25, 2017 · 10 comments
Closed

[react-native] console.group not supported #147

cparjaszewski opened this issue Oct 25, 2017 · 10 comments

Comments

@cparjaszewski
Copy link
Contributor

cparjaszewski commented Oct 25, 2017

My React-Native iOS app crashed after deploying to AppStore TestFlight because the function console.group is only supported in browser, but not in node.js.

Can you make sure the logger is turned off by default on react-native production? This is a bit frustrating I need to check it inside my app code. The library should be checking that by itself IMO.

@ttmarek
Copy link
Contributor

ttmarek commented Oct 25, 2017

Hi @KrzysztofP.

Thanks for opening an issue. Sorry for the trouble. I've made a note to improve the docs around this. Typically you wouldn't provide the logger in production. Something like this should solve your issue.

const middleware = createMiddleware(eventsMap, Target, {
  logger: __DEV__ ? logger : null,
});

I am more than welcome to a pull request to add a note about this to the docs if you are willing.

@cparjaszewski
Copy link
Contributor Author

Hi @ttmarek - sure I understand this approach, but I find this problematic to decide on logger usage in the main app code, this should be defined in the logger code itself, on production the logger should be doing nothing, but not throwing an error that crashes the app.

I also don't agree using docs is a good place for it - as the best place would be the working code of a library itself.

Imagine I'm using 10 different libraries, each of the using loggers, external services etc. all to be turned off in production - the high-level app should not depend on low level components. It's not SOLID.

@ttmarek
Copy link
Contributor

ttmarek commented Oct 25, 2017

Okay dobrze,

Let's come up with a solution then. I think there are two separate issues here.

  1. One is that the logger extension will throw errors (like the one you experienced) when it tries to use features that aren't available in its running environment (e.g. console.group).

  2. The second is that the logger extension works when in production on React Native and the web.

I don't think number 2 is really an issue or a bug. Somebody may want to use the logger to debug issues in their production build, no? If we added logic within the logger to disable it in production environments then this wouldn't be possible.

Now, I think the first issue is undesirable. We could add feature detection to ensure that everything the logger extension expects to be there, in its running environment, is available (e.g. console.x, Date, and whatever else). Similar to what we've done in the Google Analytics target to ensure it doesn't break during server side rendering.

Thoughts?

@cparjaszewski
Copy link
Contributor Author

I think what actually can make sense is to create an abstraction over console.log, console.group etc. as console.log is being used on react-native production (node environment) as well.

And have a fallback logger to console.log, when console.group or any other console.X function is not available, what do you think?

@cparjaszewski
Copy link
Contributor Author

cparjaszewski commented Oct 25, 2017

Here's Node / console docs for LTS node:
https://nodejs.org/dist/latest-v6.x/docs/api/console.html

Here's Node / console docs for Latest node (console.group added):
https://nodejs.org/api/console.html

Here's normal console docs for Web:
https://developer.mozilla.org/en-US/docs/Web/API/Console

@ttmarek
Copy link
Contributor

ttmarek commented Oct 25, 2017

Ahh I see what you mean. You're thinking we could add a polyfill so the logger will work on environments that don't have certain console methods (e.g. LTS node). Let me think for a bit.

@cparjaszewski
Copy link
Contributor Author

cparjaszewski commented Oct 25, 2017

Btw - here's how React Native handles console object and why console.log works, while console.groups throws:
[tid:com.facebook.react.JavaScript] console.group is not a function.

@ttmarek yeah, a React-Native production ready polyfill would be great as there are many React Native components in my project and only yours is throwing an error at the moment :) The worst thing is that the error is being thrown only in Release mode - which does not happen often in the typical development workflow - so it's a kind of a bug that's hard to track down and frustrating in the end.

@cparjaszewski
Copy link
Contributor Author

cparjaszewski commented Oct 25, 2017

A quick snippet for logger.js, inspired by console-polyfill, maybe this can be useful:

const groups = [];
const hr = '-'.repeat(80); // 80 dashes row line

if (!console.group) {
  console.group = function logGroupStart(label) {
    groups.push(label);
    console.log('%c \nBEGIN GROUP: %c', hr, label);
  };
}
if (!console.groupEnd) {
  console.groupEnd = function logGroupEnd() {
    console.log('END GROUP: %c\n%c', groups.pop(), hr);
  };
}
...
function logEvents(events) {

@ttmarek
Copy link
Contributor

ttmarek commented Oct 25, 2017

Sounds good @KrzysztofP. I'd accept a pull request that adds this to the logger extension if you have the time to make one.

Also, if you're in a time-crunch perhaps https://www.npmjs.com/package/patch-package might be useful until I can push a new release out.

@cparjaszewski
Copy link
Contributor Author

@ttmarek PR opened: #148, happy to see you're using prettier 👍 💯

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

No branches or pull requests

2 participants