slackapi / node-tasks-app

Tasks App is a sample Task Management app built on the Slack Platform.
MIT License
99 stars 50 forks source link

[FEATURE] Simplify the way to manage / enable listeners #83

Closed seratch closed 2 years ago

seratch commented 2 years ago

Is there an existing request for this?

Have you read our code of conduct?

Feature as a user story

I have a suggestion on the way to manage listener functions in the Node.js app. 

I see the benefit to have the stateless functions to enable listener functions to `App` instance. Thanks to that, all the listener functions are easily testable. That's really nice!

One slight concern I have on the current approach is that developers need to create/edit at least three files just for adding a new listener function. Specifically,

* To add a new action listener, you will create a new source file under `listeners/actions` and its corresponding test file
* You will add a new wrapper function in `listeners/actions/index.js`
* You will call the newly added wrapper function `app.js`

If a developer forgets to do any of the three, the listener do not work. Also, the necessity to have changes in a few files can make code reviews a bit harder.

To eliminate the pain point, I would suggest simplify the parts like this:

* https://github.com/seratch/tasks-app/blob/simplified-listener-management/nodejs/app.js#L36
* https://github.com/seratch/tasks-app/blob/simplified-listener-management/nodejs/listeners/index.js
* https://github.com/seratch/tasks-app/commit/8729026ba2b085ba4a2e2c0aab7bcfa1736a61e9

With this way, the only files you need to touch would be the newly added listener itself + its test and `listeners/actions/index.js`. All of these are the essential changes for listener addition. 

If a developer needs more granular registration methods, they can do so. But I think the show-case app can come up with as a simple approach as possible.
seratch commented 2 years ago

@colmdoyle What do you think?

colmdoyle commented 2 years ago

@misscoded - You're thinking a lot about the "default" structure for sample/template apps. Does this sound reasonable to you?

misscoded commented 2 years ago

@colmdoyle It does! This suggestion actually stemmed from a Tools conversation around the DX of templates. I'm fully in support of this and think the rationale outlined above is sound.

If folks are onboard with this, we can open a PR with the suggested enhancement and ensure that the change is reflected in the templates being created right now.

srajiang commented 2 years ago

Chiming in that I like that with the listeners setup almost completely moved out of App.js in this way, it allows App.js to be where you handle things like receiver setup, custom routes, db management and other app configuration nuance only.

colmdoyle commented 2 years ago

Sounds like consensus to me!

seratch commented 2 years ago

Thanks for your quick responses! I will come up with a pull request later.

misscoded commented 2 years ago

Fixed with #84