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

Socketmode Middleware Design Pattern #904

Merged
merged 38 commits into from Jun 2, 2022

Conversation

xNok
Copy link
Contributor

@xNok xNok commented Feb 27, 2021

I recently suggested (#899) to implement a middleware design pattern to make the use of socketmode convenient. I believe this to be such an improvement to my own code that I implemented it right away.

The basic concept behind is that it Matches incoming Event against the list of registered Event and calls all handler that are expecting that specific event. Much as you declare your routes when writing an APIs.

@kanata2
Copy link
Member

kanata2 commented Mar 16, 2021

Personally, I think it's a good idea. Let me consider whether this should be included in slack-go/slack.

@kanata2 kanata2 added experimental experimental feature and removed thinking labels Apr 20, 2021
@xNok
Copy link
Contributor Author

xNok commented Apr 25, 2021

@kanata2 I cannot reproduce the error on go 1.15. I triggered the actions on my side https://github.com/xNok/slack/actions. Maybe re-running the action may be enough

@HunterL
Copy link

HunterL commented Jun 2, 2021

Hey xNok, love using a middleware pattern here. Noticed one quick thing while going over your PR

  • The addition to examples seems to have a minor misspelling
    examples/sockermode_handler/socketmode_handler.go should be
    examples/socketmode_handler/socketmode_handler.go

Would be great to get this into master, thanks for your work

Copy link
Member

@kanata2 kanata2 left a comment

Choose a reason for hiding this comment

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

Sorry for slow response. I added some comments.

socketmode/socketmode_eventhandler.go Outdated Show resolved Hide resolved
examples/sockermode_handler/socketmode_handler.go Outdated Show resolved Hide resolved
socketmode/socketmode_eventhandler.go Outdated Show resolved Hide resolved
socketmode/socketmode_eventhandler.go Outdated Show resolved Hide resolved
socketmode/socketmode_eventhandler.go Outdated Show resolved Hide resolved
socketmode/socketmode_eventhandler.go Outdated Show resolved Hide resolved
socketmode/socketmode_eventhandler.go Outdated Show resolved Hide resolved
socketmode/socketmode_eventhandler.go Outdated Show resolved Hide resolved
socketmode/socketmode_eventhandler.go Outdated Show resolved Hide resolved
socketmode/socketmode_eventhandler.go Outdated Show resolved Hide resolved
@notwedtm
Copy link

@kanata2 is there anything that can be done to assist you in getting this PR merged in? It's been open since April :(

@kanata2
Copy link
Member

kanata2 commented Nov 2, 2021

@xNok Could you please add a note in the README that this is an EXPERIMENTAL feature?

@kanata2 kanata2 added the feedback given When a review has been conducted and awaiting the response from the comitter(s) label Nov 6, 2021
@xNok
Copy link
Contributor Author

xNok commented Nov 9, 2021

@kanata2 I added the comment in the readme

As a side note make pr-prep doesn't lint recursively. It took me a while to understand why golangci-lint was not happy.

I'll do a separate PR to fix that.

// ChannelRename is sent when a channel is rename.
ChannelRename = "channel_rename"
ChannelRename = EventsAPIType("channel_rename")
// ChannelIDChanged is sent when a channel identifier is changed.
ChannelIDChanged = "channel_id_changed"

Choose a reason for hiding this comment

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

Missed EventsAPIType^^

@mattgialelis
Copy link

Any updates on getting this merged in or anything i can help with to speed it up. @xNok @kanata2 Just would be great to be able to use the releases than tagged to a specific commit

@arulk
Copy link

arulk commented Feb 7, 2022

IS this PR going to be merged ?

@zchee zchee requested review from kanata2 and zchee February 24, 2022 14:26
@zchee
Copy link
Contributor

zchee commented Feb 26, 2022

@xNok Hi, this is newer slack-go maintainer.
I so very interesting this approach. So I'll review soon.

@kanata2 When If I done of review (and fixed any pointed out by contributor if exist), Can I merge at my discretion?

@kanata2
Copy link
Member

kanata2 commented Feb 28, 2022

@zchee
I think the problem of this thread does not resolve yet.
So I had been considering preparing a new implementation based on this PR. (Sorry for delay...)
However, if you think it is ready to be merged, you are welcome to do so.

@mattgialelis
Copy link

Just to find out whats the future of this PR if you're planning to implement something yourself @kanata2 ? And id be curious when would the new implantation be ready to use ?

As having the functionality of this or another middleware makes it a lot simpler to interact with socketmode without the need for alot of boiler plate code just to structure an event handler ?

@zchee
Copy link
Contributor

zchee commented Mar 4, 2022

@kanata2 have you any local develop branch, or something?
Also,

I think the problem of this thread does not resolve yet

Where is you said "problem"? (just for confirming

@kanata2
Copy link
Member

kanata2 commented Apr 24, 2022

I'm plan to merge this and release v0.11.0 in a few days.

@zchee
Copy link
Contributor

zchee commented May 30, 2022

@kanata2 What's the status of merge this?

@kanata2
Copy link
Member

kanata2 commented May 30, 2022

@zchee I believe it is good to merge. What do you think?

@zchee
Copy link
Contributor

zchee commented Jun 2, 2022

@kanata2 I think so, too.

@kanata2
Copy link
Member

kanata2 commented Jun 2, 2022

I'll fix linter's error in another PR.

@kanata2 kanata2 merged commit 4bbfcfb into slack-go:master Jun 2, 2022
@kanata2 kanata2 removed needs review feedback given When a review has been conducted and awaiting the response from the comitter(s) labels Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants