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

adding type as a valid constraint for app actions #326

Merged

Conversation

selfcontained
Copy link
Contributor

Summary

This adds type as an additional constraint for actions. Motivation for this is to allow slightly more flexibility for routing specific types of actions to certain handlers. Being able to constrain by type allows you to route actions that may have different types but the same action_id (or other constraint properties), or even route all actions of a specific type to one handler (regardless of callback_id, action_id or block_id.

In response to #325

both

Requirements (place an x in each [ ])

@codecov
Copy link

codecov bot commented Dec 4, 2019

Codecov Report

Merging #326 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #326   +/-   ##
=======================================
  Coverage   73.71%   73.71%           
=======================================
  Files           7        7           
  Lines         506      506           
  Branches      145      145           
=======================================
  Hits          373      373           
  Misses        101      101           
  Partials       32       32
Impacted Files Coverage Δ
src/App.ts 79.19% <100%> (ø) ⬆️

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 1d9cbaf...4c96696. Read the comment docs.

@stevengill stevengill added the enhancement M-T: A feature request for new functionality label Dec 4, 2019
Copy link
Member

@stevengill stevengill left a comment

Choose a reason for hiding this comment

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

LGTM assuming others are cool with the change in general.

@aoberoi
Copy link
Contributor

aoberoi commented Dec 11, 2019

Hi friends! Sorry for the late addition here, but I've got some input to share!

I totally agree that this is a use case worth solving. More flexibility in routing is a win.

My input is that I think we can do this in a way that adds even more value to users if we embrace the type system. For example, wouldn't it be nice if Bolt could tell you that you were doing something wrong here?

app.action({ type: 'message_action', action_id: 'my_action' }, handlerFn);

The type property indicates that you're handling a MessageAction, a type we've already invested in precisely defining, but the action_id doesn't exist on those kinds of actions!

Or what about just helping a developer understand what the options might be for the type constraint by allowing your editor's inference to give you suggestions? And once you use a specific value for the type constraint, the type inference in your listener could narrow the action argument from the catch-all SlackAction type to the very specific action (e.g. MessageAction) you constrained it to.

What this would require is instead of using string? as the type of type (whoa, confusing), using something more specific. Making this sort of change requires pretty deep familiarity with TypeScript, specifically generics, and the set of types we created in the src/types/* directory. But the good news is that most of the payloads are already there so there's no tedious work in looking up a bunch of unknowns.

I'd be happy to help make this sort of an adjustment, but I'd prefer to work with someone on it so that I'm not the only one familiar with those parts of the code. Does anyone want to work on this with me?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality semver:minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants