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

Remove references to passing port to start method when using socket mode #1202

Merged
merged 4 commits into from
Nov 9, 2021
Merged

Remove references to passing port to start method when using socket mode #1202

merged 4 commits into from
Nov 9, 2021

Conversation

filmaj
Copy link
Contributor

@filmaj filmaj commented Nov 8, 2021

In the getting started guides.

Fixes #1200.

@filmaj filmaj added the docs M-T: Documentation work only label Nov 8, 2021
@filmaj filmaj self-assigned this Nov 8, 2021
filmaj
filmaj commented Nov 8, 2021
View reviewed changes
docs/_tutorials/ja_getting_started.md Outdated
@@ -161,7 +161,8 @@ const app = new App({
token: process.env.SLACK_BOT_TOKEN,
signingSecret: process.env.SLACK_SIGNING_SECRET,
socketMode: true,
appToken: process.env.SLACK_APP_TOKEN
appToken: process.env.SLACK_APP_TOKEN,
port: process.env.PORT || 3000 // ソケットモードはポートでリッスンしませんが、アプリがOAuthに応答するようにしたい場合は、ポートでリッスンする必要があります。
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seratch please excuse my poor translation (using Google Translate) - could you take a look at this and make sure it is not embarrassingly bad? Thank you! 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

@wongjas may also be able to assist here!

@codecov
Copy link

codecov bot commented Nov 8, 2021 •
edited
Loading

Codecov Report

Merging #1202 (a448055) into main (8f5fd59) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1202   +/-   ##
=======================================
  Coverage   72.22%   72.22%           
=======================================
  Files          17       17           
  Lines        1433     1433           
  Branches      430      430           
=======================================
  Hits         1035     1035           
  Misses        322      322           
  Partials       76       76

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f5fd59...a448055. Read the comment docs.

srajiang
srajiang approved these changes Nov 8, 2021
View reviewed changes
Copy link
Contributor

@srajiang srajiang left a comment

Choose a reason for hiding this comment

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

Non-japanese parts look good!
Curious, are you using some kind of linter that's auto-removing those end-spaces?

@filmaj
Copy link
Contributor Author

filmaj commented Nov 8, 2021

Curious, are you using some kind of linter that's auto-removing those end-spaces?

No.. just my own eye-twitch-causing obsessions 🤓

misscoded
misscoded reviewed Nov 8, 2021
View reviewed changes
Copy link
Contributor

@misscoded misscoded left a comment

Choose a reason for hiding this comment

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

Noodling as I read through this: if the inclusion of a port is only relevant if the developer is dealing with OAuth, should we include the usage of port only in that section of the documentation, rather than adding it to all examples where that might not be the case?

docs/_tutorials/getting_started.md Outdated
@@ -176,7 +176,8 @@ const app = new App({
token: process.env.SLACK_BOT_TOKEN,
signingSecret: process.env.SLACK_SIGNING_SECRET,
socketMode: true,
appToken: process.env.SLACK_APP_TOKEN
appToken: process.env.SLACK_APP_TOKEN,
port: process.env.PORT || 3000 // socket mode doesn't listen on a port, but in case you want your app to respond to OAuth, you still need to listen on some port!
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this comment look in the live document? I could be totally wrong, but it seems like it will required a far scroll right to read. If so, is there a way to truncate it while keeping the same meaning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's a little screenshot from the locally running server on my Chrome:

Screen Shot 2021-11-08 at 8 12 47 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know what you think!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, on both points I entirely forgot this was strictly limited to the Getting Started layout. LGTM :shipit:

seratch
seratch reviewed Nov 8, 2021
View reviewed changes
docs/_tutorials/getting_started.md Outdated Show resolved Hide resolved
docs/_tutorials/ja_getting_started.md Outdated Show resolved Hide resolved
@filmaj
Copy link
Contributor Author

filmaj commented Nov 9, 2021

@misscoded I think it's worthwhile pointing out the need for a port in socket mode specifically to cover the OAuth use cases, even if it's not directly applicable to the specific simpler Getting Started context directly. Based on what the discussion with users was like in #1200, users are definitely just copy-pasting from the Getting Started guide to cover their initial steps in the app. I feel like including this sort of covers us in the sense that if the user ignores the two line comment, that is kind of their mistake?

What other alternatives could we consider? Remove all references of a port when it comes to the section of the Getting Started guide where we move to socket mode? I feel like that would be confusing given the very first snippet in the guide does use a port (and implicitly the HTTPReceiver under the hood).

It's not ideal whether we include port or ignore it altogether when it comes to Socket Mode... tradeoffs in both situations 🤔

@filmaj filmaj merged commit 02061d7 into main Nov 9, 2021
@filmaj filmaj deleted the getting-started-socketmode branch November 9, 2021 13:32
@seratch seratch added this to the 3.9.0 milestone Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seratch seratch seratch left review comments

@srajiang srajiang srajiang approved these changes

@misscoded misscoded misscoded approved these changes

@stevengill stevengill Awaiting requested review from stevengill

Assignees

@filmaj filmaj

Labels
docs M-T: Documentation work only
Projects
None yet
Milestone
3.9.0
Development

Successfully merging this pull request may close these issues.

Bolt App also listens to requests on port 3000 even when it's not supposed to
4 participants
@filmaj @seratch @misscoded @srajiang