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

Remove debug output to console. #112

Open
wants to merge 1 commit into
base: legacy
Choose a base branch
from
Open

Conversation

bfabio
Copy link
Member

@bfabio bfabio commented Nov 7, 2019

No description provided.

@bfabio
Copy link
Member Author

bfabio commented Nov 7, 2019

If we want a toggleable debug output in production (off by default), I can provide a patch for that.

@sebbalex
Copy link
Member

Thanks, @bfabio I think that may be useful have a toggle to switch debug on/off. Even this is not the case, there were too many console.log() abused, but some can be useful.

@libremente
Copy link
Member

Hi @bfabio and thanks for your contribution.
I would definitely insert a global debug variable, something ifndef C style, or maybe even an ENV var? I am not aware of the best practice when dealing with this situation in react, are you @sebbalex?

@bfabio
Copy link
Member Author

bfabio commented Nov 15, 2019

Before @sebbalex weighs in, here's my two cents. The way I see it is that we have two options:

We want to always turn off console output in production

This should be just a matter of using a log wrapper and set a log level based on the value of process.env.NODE_ENV.

I think we don't want a solution based around stripping the calls at build time (like a babel plugin), because it encourages spreading console.log() all over the code, with no disincentive in doing so. I think the developer should explicity call something like foobar.debug() whenever they decide it's a meaningful debug message worth committing.

We want the ability to turn on console output in production

I don't know if this makes sense for an application like publiccode-editor: it's small, simple, fast to build/deploy and it has no state, so it's rare that you need to debug something that only happens in production. BUT! In one of my project I have the code ready to toggle the debug output using the Konami code, that'd be a fun easter egg :)

@libremente
Copy link
Member

Speaking frankly, I am against having console.log() in production code. However (but this is definitely not the case for all the "legacy" prints that remained in the code analyzed in this PR), I believe that some prints might be useful in this phase where we are switching the validation engine from the internal one to the remote one (invoked via APIs). That's why I would not mind having some logs right now - they can help quickly triage and reproduce the bugs. For the rest, I strongly agree with you, we should not build any additional wrapper around them, just get rid of them all.
And LOL for the konami code, that could be serious fun 🤣

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.

3 participants