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

Revisit IncomingEventType.ViewSubmitAction #265

Closed
3 tasks done
aoberoi opened this issue Sep 28, 2019 · 5 comments
Closed
3 tasks done

Revisit IncomingEventType.ViewSubmitAction #265

aoberoi opened this issue Sep 28, 2019 · 5 comments
Labels
discussion M-T: An issue where more input is needed to reach a decision

Comments

@aoberoi
Copy link
Contributor

aoberoi commented Sep 28, 2019

Description

In the internal IncomingEventType enum, the Action value represents interactive message actions, dialog submit actions, message actions, and block actions. However, the new ViewSubmitAction is distinct.

Are we breaking a convention here? What makes the ViewSubmitAction value different enough from the other Action events?

Requirements (place an x in each of the [ ])

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've searched for any related issues and avoided creating a duplicate issue.
@aoberoi aoberoi added the discussion M-T: An issue where more input is needed to reach a decision label Sep 28, 2019
@seratch
Copy link
Member

seratch commented Sep 30, 2019

Having the action for view_submission (and missing view_closed pointed out at #263 ) in the Action family looks good to me.

@shaydewael
Copy link
Contributor

shaydewael commented Oct 1, 2019

Right now, there is an IncomingEventType for every unique method that a Bolt app has access to. This makes it easier to build the method parameters in places like https://github.com/slackapi/bolt/blob/%40slack/bolt%401.3.0/src/App.ts#L392, and makes it so we can use small helper functions like https://github.com/slackapi/bolt/blob/%40slack/bolt%401.3.0/src/App.ts#L503. There's a pretty clear isolation of code relevant to each method. We even separate file structure like types based on this logic (https://github.com/slackapi/bolt/tree/master/src/types). This is a precedent we can diverge from, but I think it'll add additional logic in places where we're separating payloads based on the method they correspond to.

If we decide to make ViewSubmission an action, we'll need to make some changes to the actions() method. We could:

  1. Change how we construct the actions() method and explicitly not accept payloads for all of the SlackActions we have defined (i.e. we wouldn't accept the view_submission payload even though we define it as an Action). This would match current-day behavior.
  2. Allow all SlackActions to be handled by the actions() method. This introduces two ways that developers could handle view submission events - views() and actions(). That's not necessarily bad, but could cause confusion.

@shaydewael
Copy link
Contributor

Stepping away from the technical side and more of a framework design choice, I think there's a pretty clear separation between block_actions and any submissions. I would argue for pulling dialog_submission events out of the categorization of actions into something like a Submission over adding to the overloaded definition that we assign to Action right now.

@seratch
Copy link
Member

seratch commented Oct 2, 2019

To understand the current status clearly, I summarized it in the following table:

Type Routing IncomingEventType
block_actions app.action(action_id, fn) Action
block_suggestion app.options(action_id, fn) Options
interactive_message app.action({ callback_id: '' }, fn) Action
message_action app.action({ callback_id: '' }, fn) Action
dialog_submission app.action({ callback_id: '' }, fn) Action
dialog_suggestion app.options({ callback_id: '' }, fn) Options
dialog_cancellation (unsupported)
view_submission app.view(callback_id, fn) ViewSubmitAction
view_closed (unsupported)

@seratch
Copy link
Member

seratch commented Mar 19, 2021

After the above conversation, we've added view_closed support by #276 . In this change, we introduced ViewClosedAction and type SlackViewAction = ViewSubmitAction | ViewClosedAction.

We may revisit some types as long as we can keep backward-compatibility but we don't have any particular plans for it at this moment. Let me close this issue now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion M-T: An issue where more input is needed to reach a decision
Projects
None yet
Development

No branches or pull requests

3 participants