[FEATURE] Simplify the way to manage / enable listeners
Closed this issue · 7 comments
Is there an existing request for this?
- I have searched for any related issues and avoided creating a duplicate issue.
Have you read our code of conduct?
- I have read and agree to the 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.@colmdoyle What do you think?
@misscoded - You're thinking a lot about the "default" structure for sample/template apps. Does this sound reasonable to you?
@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.
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.
Sounds like consensus to me!
Thanks for your quick responses! I will come up with a pull request later.