Tests

[jacekelgda]

There is one test added in this project as example. Take a look inside ./util/__tests__. We need to add unit tests in some parts of this application. There is no good idea to create tests for handlers and controllers right now so discussion is more than welcome.

[andrewgreenh]

For the controllers, I have a couple of suggestions:

• Don't add the callback function to the router in the controller file and don't export a router. I can see 2 different ways: Only export the callback functions (export const wakeUp = () => {}) this way, you can still test the function by just calling it with a mocked request object. This means, that you have a router file next to all controllers, that does the wiring. This file then has to know, which function has to be invoked on which route. The second option is, to export "annotated callbacks" objects. Each such handler object has an execute function an a route property. Some place in your application can then just loop through all annotated callbacks and add them to a router.
• The next level could then be, to export composable functions. You can take a look at this article to see what I mean: http://blog.ploeh.dk/2017/01/30/partial-application-is-dependency-injection/

[jacekelgda]

Good ideas @andreasgruenh ! Would you be willing to create a proof of concept for your suggestions ? Do you have any ideas for testing RTM API ( websockets ) for slack part ?

[andrewgreenh]

It could look something like this:

Advantages of this solution:

• Easily testable, as the dependencies (for example exchange code for token) can be easily exchanged
• These files have no dependency on express or any other concrete file in your source code
• Central composition root: routes.js decides which dependencies are used by the controllers.
• Each controller can still decide on which method it wants to listen or on which path

Disadvantages

• routes js will pull in lots of other files but has no business logic, only wire up code
• similar testability can be acchieved by overwriting the require calls (I don't really like this approach, as require calls already specify a concrete implementation) and replace the functions with mocks/spies

controllers/composeApplyToRouter.js

export default const composeApplyToRouter = ({ method, path }) => function(router) {
    router[method](path, this)
}

controllers/auth.js

import composeApplyToRouter from './composeApplyToRouter'

const composeAuth = ({
    exchangeCodeForToken,
    storeTeamToken,
    createNewBotConnection,
    sendPostInstallMessage,
}) => {
    async function auth(req, res) {
        try {
            const token = await exchangeCodeForToken(req.query.code)
            storeTeamToken(token)
            const bot = createNewBotConnection({
                token: token.bot.bot_access_token,
                team: token.team_id
            })
            sendPostInstallMessage(bot, token.user_id, token.team_id)
            res.send('Thank you for authorizing our application')
        } catch (e) {
            res.send('Error. Invalid/expired code.')
        }
    }
    auth.applyToRouter = composeApplyToRouter({ method: 'get', path: '/auth' })
    return auth
}

export default composeAuth

routes.js

import { Router } from 'express'
import { exchangeCodeForToken } from '../handlers/api'
import { storeTeamToken } from '../handlers/store'
import { createNewBotConnection, sendPostInstallMessage } from '../handlers/bot'
import composeAuth from './auth'

const router = new Router()

composeAuth({
    exchangeCodeForToken,
    storeTeamToken,
    createNewBotConnection,
    sendPostInstallMessage
}).applyToRouter(router)

[RafalWilinski]

Great suggestions @andreasgruenh, thanks for valuable input! 💯

Your suggestion definitely makes sense, I'm also not a big fan of overwriting require calls so I'd rather proceed with the flow proposed by you.

Regarding the first disadvantage - I don't think that putting that code into router is a big issue.