Skip to content

chore: remove usage of console.log in primary SPA#2148

Merged
chrismclarke merged 3 commits intomasterfrom
chore/remove-console-log
Mar 22, 2023
Merged

chore: remove usage of console.log in primary SPA#2148
chrismclarke merged 3 commits intomasterfrom
chore/remove-console-log

Conversation

@thisislawatts
Copy link
Contributor

@thisislawatts thisislawatts commented Mar 19, 2023

PR Checklist

PR Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Developer experience (improves developer workflows for contributing to the project)

Description

  • chore: prevent console log
  • chore: code changes to introduce logger instead of using console.log

@AlfonsoGhislieri raised a really useful bit of PR feedback around console.log. I propose we adopt automated checks to catch this and free up our reviewers time to concentrate on details not solved with tooling.


What happens next?

Thanks for the contribution! We try to make sure all PRs are reviewed ahead of a monthly dev call (first Monday of the month, open to all!).

If the PR is working as intended it'll be merged and included in the next platform release, if not changes will be requested and re-reviewed once updated.

If you need more immediate feedback you can try reaching out on Discord in the Community Platform development channel.

@thisislawatts thisislawatts requested a review from a team as a code owner March 19, 2023 18:19
@thisislawatts thisislawatts force-pushed the chore/remove-console-log branch from 4639f08 to 7c148ac Compare March 19, 2023 18:33
@thisislawatts thisislawatts mentioned this pull request Mar 19, 2023
9 tasks
@thisislawatts thisislawatts force-pushed the chore/remove-console-log branch from 7c148ac to 8430928 Compare March 19, 2023 18:47
@cypress
Copy link

cypress bot commented Mar 19, 2023

2 flaky tests on run #3066 ↗︎

0 78 3 0 Flakiness 2

Details:

Merge branch 'master' into chore/remove-console-log
Project: onearmy-community-platform Commit: 842027804b
Status: Passed Duration: 03:39 💡
Started: Mar 22, 2023 1:34 PM Ended: Mar 22, 2023 1:38 PM
Flakiness  common.spec.ts • 1 flaky test • ci-chrome

View Output Video

Test Artifacts
[Common] > [User Menu] > [By Authenticated] Output Screenshots
Flakiness  events.spec.ts • 1 flaky test • ci-chrome

View Output Video

Test Artifacts
[Events] > [Create an event] > [By Authenticated] Output Screenshots

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

Copy link
Contributor

@AlfonsoGhislieri AlfonsoGhislieri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me 👍🏻, seems like a great change to prevent these rogue console.log's making into production!

Question: All of these should now be showing up in the logflare logs?

@thisislawatts
Copy link
Contributor Author

@AlfonsoGhislieri, at the moment I do not think Logflare (or another aggregated logging tool) has been configured for any of the production instances.

Removing the console output here is the primary outcome of this change, hooking up the platform instances to Logflare will need to be done separately by someone with edit access to CircleCI as the relevant variables, REACT_APP_LOGFLARE_KEY,REACT_APP_LOGFLARE_SOURCE; are injected during the deployment process.

@thisislawatts thisislawatts force-pushed the chore/remove-console-log branch from 8430928 to 39abc72 Compare March 21, 2023 19:48
@chrismclarke
Copy link
Member

chrismclarke commented Mar 22, 2023

Thanks @thisislawatts and @AlfonsoGhislieri

If I remember correctly last time I tried looking into logflare integration there was an issue with pino-logflare compatibility for webpack 5 which we had just updated to, but I think that was fixed in #2067 so could be worth a revisit. We also have sentry and firebase crashlytics available for logging endpoints, although typically neither are actively monitored.

I'd probably suggest what would be most useful is a way to override at runtime for cases where trying to quickly debug an issue in production (e.g./how-to?logger=console) as I think we're more likely to do that than sift through combined logs.

But for now moving things over to a logger makes any transition easier in the future as we can just configure transports directly there.

@@ -0,0 +1,5 @@
{
"rules": {
"no-console": "error"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question(non-blocking)
Do you know if there's any way to add a custom message to these errors, like in https://eslint.org/docs/latest/rules/no-restricted-imports ? I'm not aware of any myself, just think might be nice to advise developers something along the lines of "Should use logger instead of console" to make clear. Not super important, just a thought.

@chrismclarke chrismclarke self-requested a review March 22, 2023 12:03
Copy link
Member

@chrismclarke chrismclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added one non-blocking question inline but otherwise all looks good to me also, many thanks

@chrismclarke chrismclarke merged commit 84c10e8 into master Mar 22, 2023
@chrismclarke chrismclarke deleted the chore/remove-console-log branch March 22, 2023 13:39
@thisislawatts
Copy link
Contributor Author

Thanks @chrismclarke

@cypress cypress bot mentioned this pull request Mar 22, 2023
@onearmy-bot
Copy link
Collaborator

🎉 This PR is included in version 1.41.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants