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

Fixes #1477 Add customPropertiesExtractor to receiver options docs #1864

Merged
merged 4 commits into from
Jun 14, 2023

Conversation

mlauter
Copy link
Contributor

@mlauter mlauter commented Jun 8, 2023

Summary

Fixes #1477 Add customPropertiesExtractor to receiver options docs

Slightly rearranges existing Custom receiver concepts section to include an example of using customPropertiesExtractor.

Details

This may be over the top relative to what you all had in mind. It seemed nice to include a usage example, and I couldn't think where else to put it.

I worried that it might be confusing to include this example in the section on writing a custom receiver, so I tried to edit it to make it clear. I left the writing a custom receiver section first because the main example on the side demonstrates writing your own custom receiver.

Requirements (place an x in each [ ])


---

#### Customizing built-in receivers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be better to move this into its own concepts page?

Copy link
Contributor

Choose a reason for hiding this comment

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

No I think this is great where it is!

@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #1864 (8810195) into main (9f171b8) will not change coverage.
The diff coverage is n/a.

❗ Current head 8810195 differs from pull request most recent head 2453300. Consider uploading reports for the commit 2453300 to get more accurate results

@@           Coverage Diff           @@
##             main    #1864   +/-   ##
=======================================
  Coverage   82.18%   82.18%           
=======================================
  Files          18       18           
  Lines        1521     1521           
  Branches      436      436           
=======================================
  Hits         1250     1250           
  Misses        175      175           
  Partials       96       96           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@filmaj filmaj added the docs M-T: Documentation work only label Jun 8, 2023
@filmaj filmaj added this to the 3.13.2 milestone Jun 8, 2023
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Hey thanks so much for the PR! This is looking great. I left a few suggestions but this is a huge improvement - many thanks for taking this on.
Feel free to re-request a review directly from me if you want me to review it again.
Cheers!

@@ -6,6 +6,9 @@ order: 9
---

<div class="section-content">

#### Writing a custom receiver

A receiver is responsible for handling and parsing any incoming requests from Slack then sending it to the app, so that the app can add context and pass the request to your listeners. Receivers must conform to the Receiver interface:
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this isn't exactly part of your PR, but perhaps we can link Receiver interface to https://github.com/slackapi/bolt-js/blob/main/src/types/receiver.ts#L27-L31?

Suggested change
A receiver is responsible for handling and parsing any incoming requests from Slack then sending it to the app, so that the app can add context and pass the request to your listeners. Receivers must conform to the Receiver interface:
A receiver is responsible for handling and parsing any incoming requests from Slack then sending it to the app, so that the app can add context and pass the request to your listeners. Receivers must conform to the [Receiver interface](https://github.com/slackapi/bolt-js/blob/main/src/types/receiver.ts#L27-L31):

Copy link
Member

Choose a reason for hiding this comment

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

@filmaj We should avoid pointing code lines in the main branch as the code can be changed in the future. How about using a release tag instead? https://github.com/slackapi/bolt-js/blob/%40slack/bolt%403.13.1/src/types/receiver.ts#L27-L31


---

#### Customizing built-in receivers
Copy link
Contributor

Choose a reason for hiding this comment

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

No I think this is great where it is!

@@ -98,6 +98,7 @@ Bolt includes a collection of initialization options to customize apps. There ar
| `unhandledRequestHandler` | Error handler triggered when a request from Slack goes unacknowledged. More details available in the [Error Handling documentation](/bolt-js/concepts#error-handling). |
| `unhandledRequestTimeoutMillis` | How long to wait, in milliseconds, from the time a request is received to when the `unhandledRequestHandler` should be triggered. Default is `3001`. More details available in the [Error Handling documentation](/bolt-js/concepts#error-handling). |
| `signatureVerification` | `boolean` that determines whether Bolt should [verify Slack's signature on incoming requests](https://api.slack.com/authentication/verifying-requests-from-slack). Defaults to `true`. |
| `customPropertiesExtractor` | Optional `function` that can extract custom properties from a request -- for example, extracting custom headers to propagate to other services. More details available in the [Customizing a receiver documentation](/bolt-js/concepts#receiver). |
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's clarify two things:

  • what the function argument is (at least, what type it is: for HTTP-based receivers, it would be an HTTP request, but for e.g. Socket Mode, it would be a WebSocket message.
  • what the function needs to return, and then how those custom properties are available in the context of a wider app. If I understand correctly, the returned object becomes part of the context argument provided to Bolt middleware.

Copy link
Member

@seratch seratch Jun 8, 2023

Choose a reason for hiding this comment

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

Indeed, Fil's points can be mentioned quickly in the descrption.

In addition to that, I think we can remove the following part because the linked document section is about how to build your own receiver, not how to customize a built-in one.

More details available in the Customizing a receiver documentation.

... Ah never mind about my point. This PR is goind to add sub section for it.


##### Extracting custom properties

Use the `customPropertiesExtractor` option to extract custom properties from requests. This is particularly useful for extracting HTTP headers that you want to propagate to other services, for example, if you need to propagate a header for distributed tracing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should add a note here that, depending on what kind of receiver you are building, or which pre-existing receiver you are extending with customPropertiesExtractor, the "from requests" part may not be exactly technically correct. For HTTP-based receivers, they would indeed be an HTTP request, but for socket-mode-based receivers, the argument to the extractor would be a websocket message sent from the server.

Copy link
Member

Choose a reason for hiding this comment

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

Checking examples/custom-properties code should be helplful to understand the difference between the two (HTTP vs Socket Mode)

@mlauter
Copy link
Contributor Author

mlauter commented Jun 13, 2023

@seratch @filmaj thanks for your feedback, sorry got busy at work but I will try to address the feedback this week

@mlauter mlauter requested a review from filmaj June 14, 2023 01:11
@mlauter
Copy link
Contributor Author

mlauter commented Jun 14, 2023

@seratch @filmaj thanks for your feedback, sorry got busy at work but I will try to address the feedback this week

Updated in response to feedback. I worry its gotten a little wordy now, but let me know what you think! Thank you for your patience.

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.

LGTM; @filmaj feel free to merge this tomorrow if it looks great to you too

@mlauter
Copy link
Contributor Author

mlauter commented Jun 14, 2023

meant to turn off my formatter to avoid those whitespace changes and then forgot, i can remove them if it's an issue

@filmaj filmaj merged commit 0cc0caa into slackapi:main Jun 14, 2023
@filmaj
Copy link
Contributor

filmaj commented Jun 14, 2023

Thanks so much for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:signed docs M-T: Documentation work only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add customPropertiesExtractor to the documents
3 participants