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.

Sign up for GitHub

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

Jump to bottom

ConsoleLogger: bind console.debug for logger.debug #12690

Merged
merged 12 commits into from
Mar 12, 2024
Merged

ConsoleLogger: bind console.debug for logger.debug #12690

merged 12 commits into from
Mar 12, 2024

Conversation

sjdeak
Copy link
Contributor

@sjdeak sjdeak commented Dec 8, 2023

Description of changes

Issue #, if available

  • Current behavior: When using consoleLogger.debug, console.log is used under the hood.
  • Issue: this causes chrome devtool's log filtering function not work. During development I usually turned on all log level in ConsoleLogger and use Chrome devtool to filter logs.

CleanShot 2023-12-08 at 22 16 09

  • Solution: bind console.debug

Checklist

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sjdeak sjdeak requested review from a team as code owners December 8, 2023 14:19
@cwomack cwomack added external-contributor first-time-contributor The contribution is the first for this user in the repo labels Jan 10, 2024
@cwomack
Copy link
Member

cwomack commented Jan 30, 2024

Hey, @sjdeak 👋. Apologies for the delayed response on this PR, but thank you for your contribution! We're going to be testing this PR and considering it for merging, so we'll follow up with any questions or updates very soon. Thank you for your patience.

@sjdeak
Copy link
Contributor Author

sjdeak commented Feb 7, 2024

@ashwinkumar6 Let me know your suggestion to this change, willing to contribute.

ashwinkumar6
ashwinkumar6 reviewed Feb 8, 2024
View reviewed changes
Copy link
Member

@ashwinkumar6 ashwinkumar6 left a comment

Choose a reason for hiding this comment

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

Hey @sjdeak,
Thanks for raising the PR, appreciate the contribution. I've added in a minor suggestion to also bind console.info

packages/core/src/Logger/ConsoleLogger.ts Outdated Show resolved Hide resolved
@ashwinkumar6
Copy link
Member

To unblock CI could you please add in these changes as well.

@sjdeak
Copy link
Contributor Author

sjdeak commented Feb 9, 2024

Thanks! It's traditional festival in my country will soon response once when I'm back to work.

@sjdeak sjdeak requested a review from a team as a code owner February 9, 2024 18:10
ashwinkumar6
ashwinkumar6 previously approved these changes Feb 9, 2024
View reviewed changes
ashika112
ashika112 previously approved these changes Feb 10, 2024
View reviewed changes
ashwinkumar6
ashwinkumar6 previously approved these changes Feb 12, 2024
View reviewed changes
@ashwinkumar6
Copy link
Member

Hey @sjdeak,
We're performing some additional validations from our side, Thank you for your patience.

@ashwinkumar6 ashwinkumar6 dismissed stale reviews from ashika112 and themself via 3175cd8 February 16, 2024 20:47
ashwinkumar6
ashwinkumar6 previously approved these changes Feb 16, 2024
View reviewed changes
ashika112
ashika112 previously approved these changes Feb 16, 2024
View reviewed changes
@sjdeak sjdeak dismissed stale reviews from ashika112 and ashwinkumar6 via 3efa65e February 19, 2024 09:32
@sjdeak
Copy link
Contributor Author

sjdeak commented Feb 19, 2024

Hi @ashwinkumar6 @ashika112 , has addressed comments and fixed the failed test case(also verified locally), please check, thanks.

@ashwinkumar6 ashwinkumar6 dismissed stale reviews from ashika112, jimblanc, and themself via c0479e9 February 21, 2024 17:02
@sjdeak
Copy link
Contributor Author

sjdeak commented Feb 24, 2024

Hi @ashwinkumar6 could we merge the PR in next week? Really looking forward to see a cleaner console log when using Amplify, thanks!

@ashwinkumar6
Copy link
Member

Thanks for your patience @sjdeak. We're exploring making this update an opt-in feature to avoid any breaking changes, For example we could introduce a property to toggle for users to enable at their discretion. I'm currently away and will be back in a week. Once I return, I'll work on finalizing this approach.

@sjdeak
Copy link
Contributor Author

sjdeak commented Feb 26, 2024

Understood. thank you

david-mcafee
david-mcafee previously approved these changes Mar 11, 2024
View reviewed changes
Copy link
Contributor

@david-mcafee david-mcafee left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

ashika112
ashika112 previously approved these changes Mar 11, 2024
View reviewed changes
@ashika112 ashika112 self-requested a review March 11, 2024 21:22
@ashwinkumar6 ashwinkumar6 dismissed stale reviews from ashika112 and david-mcafee via a0be4bb March 12, 2024 18:02
ashika112
ashika112 approved these changes Mar 12, 2024
View reviewed changes
Copy link
Member

@ashika112 ashika112 left a comment

Choose a reason for hiding this comment

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

Thank you 👍 Ship it!!

@ashwinkumar6 ashwinkumar6 merged commit 8bb0db9 into aws-amplify:main Mar 12, 2024
30 checks passed
@ashwinkumar6
Copy link
Member

Hi @sjdeak,
This has been released in 6.0.21

@sjdeak
Copy link
Contributor Author

sjdeak commented Mar 19, 2024 •
edited
Loading

Hi @sjdeak,

This has been released in 6.0.21

Great, @ashwinkumar6 thank you very much ! it is such a great experience work with you team.

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

@ashika112 ashika112 ashika112 approved these changes

@david-mcafee david-mcafee david-mcafee approved these changes

@jimblanc jimblanc Awaiting requested review from jimblanc

@ashwinkumar6 ashwinkumar6 Awaiting requested review from ashwinkumar6

Assignees

@ashwinkumar6 ashwinkumar6

@cwomack cwomack

Labels
external-contributor first-time-contributor The contribution is the first for this user in the repo
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sjdeak @cwomack @ashwinkumar6 @jimblanc @david-mcafee @ashika112