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

Disable console logging by default #268

Merged

Conversation

sjdemartini
Copy link
Contributor

Description
Turns off logging to the console by default. Logging can still be re-enabled by calling adapter.disableLog(false).
Resolves https://github.com/webrtc/adapter/issues/68.

Purpose
Logging by default is not desirable functionality. For an example of not logging to the console being standard convention, see this linting rule from ESLint: http://eslint.org/docs/rules/no-console. Here is an excerpt:

In JavaScript that is designed to be executed in the browser, it’s considered a best practice to avoid using methods on console. Such messages are considered to be for debugging purposes and therefore not suitable to ship to the client. In general, calls using console should be stripped before being pushed to production.

As such, having logging enabled by default in adapter.js (which therefore logs the first "switch" statement of code in the browser, without the ability to turn it off) is fairly bad non-standard practice, and it requires editing the source or forking the project before using the code in production. The adapter code should be as widely usable as possible (and of course designed for the browser), so logging should be off by default.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@sjdemartini sjdemartini force-pushed the disable-logging-by-default branch from ea94823 to 4cb64e1 Compare April 15, 2016 08:06
@sjdemartini
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@fippo
Copy link
Member

fippo commented Apr 17, 2016

this was supposed not to log when used as a module but that seems to have broken recently.
PR is LGTM

@KaptenJansson
Copy link
Collaborator

LGTM

@KaptenJansson KaptenJansson merged commit 8a53198 into webrtcHacks:master Apr 18, 2016
@sjdemartini sjdemartini deleted the disable-logging-by-default branch April 18, 2016 18:45
@sjdemartini
Copy link
Contributor Author

Thanks for the quick review and merge @fippo and @KaptenJansson! Do you know when the next release is expected to be cut?

@KaptenJansson
Copy link
Collaborator

I will try to publish it today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants