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

feat: log info and errors using the @slack/logger package #84

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

zimeg
Copy link
Member

@zimeg zimeg commented Aug 23, 2024

Type of change

  • New sample
  • New feature
  • Bug fix
  • Documentation

Summary

This PR uses @slack/logger - both instantiated and in listeners - as an interface to the console to avoid linting errors and respond to logger.setLevel changes

Warning:   66:5  warning  Unexpected console statement  no-console

Reviewers

Setup an app and inspect the logs 🔍

  $ npm run start
  ...
  [DEBUG]  web-api:WebClient:1 apiCall('apps.connections.open') end
  [DEBUG]  socket-mode:SocketModeClient:0 Transitioning to state: connecting:authenticated
- ⚡️ Bolt app is running!
+ [INFO]   ⚡️ Bolt app is running!
  [DEBUG]  web-api:WebClient:0 http response received

Notes

If this seems like a solid change, I can copy it over to other Bolt JS samples!

Requirements

  • I’ve checked my submission against the Samples Checklist to ensure it complies with all standards
  • I have ensured the changes I am contributing align with existing patterns and have tested and linted my code
  • I've read and agree to the Code of Conduct

@zimeg zimeg added enhancement New feature or request dependencies labels Aug 23, 2024
@zimeg zimeg self-assigned this Aug 23, 2024
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Thanks for the suggestion; this looks like a good first step but wanted to mention a potential change on the bolt-js side; Thoughts?

@@ -61,10 +62,11 @@ registerListeners(app);

/** Start Bolt App */
(async () => {
const logger = new ConsoleLogger();
Copy link
Member

@seratch seratch Aug 23, 2024

Choose a reason for hiding this comment

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

This is at least better than now, but we may want to expose app.logger and use it here. With that, developers can control the logging level consistently

Copy link
Member Author

Choose a reason for hiding this comment

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

🤩 I think that'd be most ideal! I agree that setting these levels in separate place isn't the best experience at the moment...

I'm also realizing this logger should be set to debug to match the app anyways!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants