-
Notifications
You must be signed in to change notification settings - Fork 21
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
chore: Adds vite-remove-console plugin #1140
Conversation
Signed-off-by: John Cowen <john.cowen@konghq.com>
✅ Deploy Preview for kuma-gui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Confused about this. We should talk more |
One approach to answer the logging question would consist of two things:
Example for an unforeseen error scenario: Today, we have a couple of places where code like this can be found: try {
// ...
} catch (err) {
if (err instanceof Error) {
error.value = err
} else {
console.error(err)
}
} It’s almost guaranteed that We should likely replace |
I think I 100% agree with what @kleinfreund is explaining here! Disallow console.log and console.error with linters and when we need to log log with the logger so when this happens in SaaS we get the info. |
Oh hey, sorry I thought I'd made a comment here already 🤦
I floated disallowing console.logs with linting when I first got here (it's what I personally am used to), and from what I remember there was a bit of pushback:
Since then we had the issue with the console logs being in the prod build. I figured this was an alternative that folks might not pushback on, sounds like opinion has changed @kleinfreund ? If so, more than happy for us add linting instead if it's no longer an issue. As for errors, its slightly different as we mostly want errors left in, so its kind of the opposite of the situation with logging:
The argument for using The other thing to take into account (as I explain in the description) is that I personally feel that our users being able to open the JS console and see errors is really handy when it comes to reporting problems or even resolving their own user issues without needing to file an issue with us to find out they'd been holding something wrong. I know this opinion is controversial, so understand if folks aren't keen on leaving them in. I think the other important thing is, even for me whilst working on the project I quite often hit something where it would be handy to see the actual error in console, but because we swallow errors all the time to report them with a very bare bones error GUI page that just says 'Error', it can be really hard out to figure out what the issue is. So to sum up this evening, right now I wholeheartedly agree with linting for logs, not totally sure about errors still. I'll sleep on it and come back tomorrow, when we can either close this or discuss a little further. Sound good? |
I want to make a clear distinction between "allowing
I don’t see the problem with that.
If we want this, the solution would be simply to not disable the logger service in production specifically for error and warn type logs. |
This was specifically what I was suggesting we do when we originally spoke (if you remember I wanted to add I think the most important thing is, it now sounds like we want exactly the same thing. So probably best to move on from there. Lets go with allowing console's locally, but adding commit hooks (or at the very least CI) to prevent accidental logging happening again. I would very much rather do it with two separate files instead so its clear where to add things if we need to do more things like this (similar to the On the errors:
If we are leaving errors in all the time (we I would suggest we do), then why would you not just use console.error instead? I think I'm going to close this PR and replace it with an issue for adding 2 configuration of linting, i.e. we can use log in dev but not in prod. Prod should be enforced either on commit with a commit hook (preferably) or at the very least in CI. Error we can continue discussion, its not as pressing as making sure we never leave console.logs in the prod build again (which was the intention with this PR) Let me know if that sounds ok to you and I'll close this up and go ahead and make an issue. |
Because going through the logger service allows us to hook those up to monitoring services more easily.
Fine by me. |
I thought console.errors end up in monitoring services also without us doing anything extra? Is that not the case? |
That should be the case, but I rather use the logger service as an explicit source for this. |
Ok so if it is the case that
I still can't see a good reason not just just use |
Closing in favour of using environment aware linting, which is what we all would prefer. The console.error discussion is still in progress and I don't want it to block adding something for logging. |
xref #1179 |
I've had second thoughts on this and would like to reopen this or a similar PR (now or at a later date, I don't mind) TLDR; Linting is to prevent us from accidentally adding Just to be absolutely clear, I really really want to add environment aware linting and I've made an issue to cover that. The thing is, I think this PR has been misled into a discussion about how we prevent accidental addition of console logs, and that is not what this PR was about. Let me try to explain: A few weeks back we changed our logging functionality to remove the DataDog integration, and replaced it with what was supposed to be development time only logging. If we'd have had linting at the time, these console logs would still have been added with lint ignores as we were adding them on purpose. Unfortunately the logging was implemented completely in reverse, i.e. we were logging only in production instead of only in development. We spotted this bug too late, and the console logs ended up in the production binary. We pushed a fix but we specifically and importantly (for the context of this PR) wanted to prevent this from ever happening again, we specifically said we should prevent this from happening again. In my eyes if we want to prevent this from happening again we can:
I prefer point 2. As long as the plugin works as advertised, it is impossible for explicitly added console.logs to end up in production. With point 3, we might get something wrong again, or not test a certain code path. I do not want to do point 1. I hope folks can understand the background to this PR. Please let me know you thoughts, or if you want any clarification. If at the end we want to go with point 1, thats fine with me as long as its not only my decision. |
Can you give an example of a log you'd only want at dev time? Also just to make sure does our logger have a "debug" level? |
This PR removes any
console.log/info/warn/errors
from any production builds (this includes our e2e testing). These calls are kept when in development, including the PR preview sites.Currently these calls are also kept during CLI testing as they don't use
vite
as yet, but it would nice to remove them from there also so we can just use them for development purposes.Note: I want us to keep removing/avoiding console.logs in code/git overall, as a rule we should not leave any of these things in the code at all. But sometimes/occasionally its useful to have
console.error/info/warn
during development in code/git. It also helps if anyconsole.logs
somehow find their way into production code.Personally I'm unsure whether we should remove
console.error
at all as they can be useful to users when hitting a problem with the application, especially if our in-app UI error pages are not super explanatory, so happy to hear anyones thoughts on that.I'm adding the plugin now, but we can always remove the
error
removal later if its not what we want.Signed-off-by: John Cowen john.cowen@konghq.com