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

feat: make the app logger attached to the app instance public #2365

Merged
merged 5 commits into from
Dec 17, 2024
Merged

feat: make the app logger attached to the app instance public #2365

merged 5 commits into from
Dec 17, 2024

Conversation

zimeg
Copy link
Member

@zimeg zimeg commented Dec 11, 2024

Summary

This PR adds the logger instance to the app for logging outside of function listeners, as this comment suggests:
slack-samples/bolt-js-starter-template#84 (review) 👾 ✨

Preview

Updated app code can use the app.logger functions once the App is constructed:

const app = new App({
  signingSecret: process.env.SLACK_SIGNING_SECRET,
  token: process.env.SLACK_BOT_TOKEN,
});

(async () => {
  await app.start(process.env.PORT);
  app.logger.info('⚡️ Bolt app started');
})();

Before changes

Using console.log instead of app.logger.info prints the following:

$ node app.js
⚡️ Bolt app started

After changes

$ node app.js
[INFO]  bolt-app ⚡️ Bolt app started

Documentation

Read more

docs

Notes

  • Updates to the example apps are left unchanged in this PR but are planned for a follow up 🙏
  • This is marked as typescript specific since before changes javascript apps could still use app.logger! The change from this PR removes errors if app.logger is used when running tsc 🧱

Requirements

@zimeg zimeg added enhancement M-T: A feature request for new functionality docs M-T: Documentation work only semver:minor TypeScript-specific labels Dec 11, 2024
@zimeg zimeg added this to the 4.2.0 milestone Dec 11, 2024
@zimeg zimeg requested review from seratch and a team December 11, 2024 03:45
@zimeg zimeg self-assigned this Dec 11, 2024
@codecov Codecov
Copy link

codecov bot commented Dec 11, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.59%. Comparing base (c091510) to head (5d33df4).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #2365   +/-   ##
=======================================
  Coverage   92.59%   92.59%           
=======================================
  Files          36       36           
  Lines        7472     7472           
  Branches      653      653           
=======================================
  Hits         6919     6919           
  Misses        545      545           
  Partials        8        8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

seratch
seratch approved these changes Dec 11, 2024
View reviewed changes
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 working on this! Looks good to me, but you may want to wait for others' feedback for one or two business days. Also, this PR includes document changes too, so we should release a new version including the change as early as possible once it's merged.

zimeg
zimeg commented Dec 11, 2024
View reviewed changes
Copy link
Member Author

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

📝 Notes of the changes for the kind reviewers!

src/App.ts
@@ -222,7 +222,7 @@ export default class App<AppCustomContext extends StringIndexed = StringIndexed>
private receiver: Receiver;

/** Logger */
private logger: Logger;
public logger: Logger;
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the update for tsc! 🎉

docs/content/reference.md
@@ -48,6 +48,7 @@ Listener functions have access to a set of arguments that may change based on th
| `respond` | `action`, `shortcut`, `view`, `command` | Function that responds to an incoming event **if** it contains a `response_url`. `respond` returns a promise that resolves with the results of responding using the `response_url`. For shortcuts, `respond` will **only** work for message shortcuts (not global shortcuts). For views, `respond` will **only** work when using `response_url_enabled: true` for [conversations list](https://api.slack.com/reference/block-kit/block-elements#conversation_select) and [channels list](https://api.slack.com/reference/block-kit/block-elements#channel_select) select menus in input blocks in modals. |
| `context` | All listeners | Event context. This object contains data about the event and the app, such as the `botId`. Middleware can add additional context before the event is passed to listeners. |
| `body` | All listeners | Object that contains the entire body of the request (superset of `payload`). Some accessory data is only available outside of the payload (such as `trigger_id` and `authorizations`). |
| `logger` | All listeners | The application logger with all of [the logging functions](/concepts/logging) for output. |
Copy link
Member Author

Choose a reason for hiding this comment

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

Super open to suggestions here! But this is what's shown now:

table

WilliamBergamin
WilliamBergamin approved these changes Dec 11, 2024
View reviewed changes
Copy link
Contributor

@WilliamBergamin WilliamBergamin left a comment

Choose a reason for hiding this comment

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

Looks good to me as well 💯 seem like this was originally set to private and never revisited

This behavior also aligns with the bolt python behavior

@zimeg
Copy link
Member Author

zimeg commented Dec 11, 2024

@seratch @WilliamBergamin Thank y'all both for reviewing! I'm tagging @lukegalbraithrussell here for awareness on updates to docs, and will plan on merging this right before the next release is tagged - good catch with the docs differences that'd appear with an earlier merge! 📚 ✨

@zimeg zimeg mentioned this pull request Dec 17, 2024
Merged
3 tasks
@zimeg
Copy link
Member Author

zimeg commented Dec 17, 2024

@seratch @WilliamBergamin @lukegalbraithrussell thank y'all all for the reviews! I'm glad that we find another match between js and py with these changes 🐍

We're set to release this in the next version with #2371 and testing continues to pass as expected so I think now's a good time to merge! 🚢 💨 I'll begin the updates to included examples for another PR 🔍

@zimeg zimeg merged commit b6caa0a into main Dec 17, 2024
20 checks passed
@zimeg zimeg deleted the zimeg-feat-app-logger branch December 17, 2024 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukegalbraithrussell lukegalbraithrussell lukegalbraithrussell approved these changes

@WilliamBergamin WilliamBergamin WilliamBergamin approved these changes

@seratch seratch seratch approved these changes

Assignees

@zimeg zimeg

Labels
docs M-T: Documentation work only enhancement M-T: A feature request for new functionality semver:minor TypeScript-specific
Projects
None yet
Milestone
4.2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@zimeg @seratch @WilliamBergamin @lukegalbraithrussell