ryanvolum / offline-directline

Unofficial package to emulate the bot framework connector locally.
https://www.npmjs.com/package/offline-directline
78 stars 44 forks source link

Let consumer control server listening #27

Closed mattmazzola closed 6 years ago

mattmazzola commented 6 years ago

The control of server should exist in single layer. Since initializeRoutes is expecting the consumer to create and provide a server, the consumer should also be in control of invoking listening

This allows consuming library to setup server with directline without listening on that port immediately. It also allows them to understand how to start multiple instances of direct line implicitly based on their knowledge of using express instead of having to pass the port in and guessing about internals of offline-direct

const port = process.env.PORT || 3000
const server = express()
directline.initializeRoutes(server, `http://127.0.0.1:${port}`, botUrl)
const listener = server.listen(port, () => {
    console.log(`Server listening to ${listener.address().port}`)
})

I think an even better solution would be to use express.Router and return an instance for them to use https://expressjs.com/en/4x/api.html#router

const port = process.env.PORT || 3000
const server = express()
const directlineRouter = directline.createRouter(`http://127.0.0.1:${port}`, botUrl)
server.use('/directline', directlineRouter)
const listener = server.listen(port, () => {
    console.log(`Server listening to ${listener.address().port}`)
})
mattmazzola commented 6 years ago

For specific use case: We want to add these directline routes to an existing server which already has it's own code for listening. However, if we use directline we now have to control whether we can call our own listen or use the one inside directline to avoid conflicts. The one inside directline also doesn't print the same information in the callback or allow customizing so it adds unnecessary complexity.

mattmazzola commented 6 years ago

I think there is way to do this in non-breaking way. I would add expose new method from library called createRouter which returns instance of express.Router. The old initializeRoutes would simply use the router. People can opt-in to only using createRouter instead of initializeRoutes if they want the new behavior.

If you think this is acceptable idea, let me know.