cul-2016 / quiz

11 stars 4 forks source link

Rearchitecure the server routes to be grouped as plugins #297

Open samhstn opened 7 years ago

samhstn commented 7 years ago

Every endpoint should have it’s rightful place in the codebase So far that place has been the handlers directory, but that has lead to the handlers directory having a flat structure containing 29 files. Grouping the endpoints in plugins to me made sense and has little overhead. I’m open to changing this. Although the main reason for the grouping was to be able to refactor the code and I find that moving things around is easier in one file than when things are spread across multiple files.

Problem No clear grouping of routes is making it more difficult to refactor (abstract out repeated code). No grouping makes it harder to see what routes are linked to each other Solution Grouping the routes as plugins will allow for common helpers to be required into the relevant plugins

Problem We are currently starting up a database and then requiring it in every endpoint that requires access to a db Eg. the getUser helpers in the users.js plugin Solution As I have seen in hapi-redis-connection and hapi-postgres-connection implementations, it is good to attach the db client to an object which can be accessed by every route. Plugins give access to the server.app object which is where the clients will be attached.

You can see the benefit of this grouping and modularisation in the endpoints.test.js file which summarises all of the previous endpoint tests.

The tests have been made easier because when testing an endpoint we can simply require in the server and access the clients instead of requiring in 3 files (server, redis, pg).

I am unsure of any issues with architecturing the server to group routes in plugins as mentioned above but we would love to hear your feedback (@nelsonic @Danwhy @iteles).

We are keen to not prioritise the refactoring until it becomes some kind of blocker, but this new architecture may allow for smoother refactoring which we will be tackling when we implement server auth #284

sohilpandya commented 7 years ago

@shouston3 @Danwhy here is where the changes have been made for this issue - https://github.com/cul-2016/quiz/pull/298/files#diff-c9bb31bbcc08d333b150cdd7e696ad9e

Danwhy commented 7 years ago

I think grouping related endpoints/routes together makes sense, but I'm not sure about having them all in one file.

I think the best way would be to group them all into directories, and require the individual routes into an index.js file in that directory. That way you've got them grouped together, but still modular, so you can test individual functions easily, and avoid merge confilcts.

nelsonic commented 7 years ago

@shouston3 for a small project having the routes in a single file is fine. As soon as we have many routes the routes.js file will be updated constantly which will lead to merge conflicts. @Danwhy's suggestion based on the experience of https://github.com/TheScienceMuseum/collectionsonline/blob/master/routes/index.js seems sensible. 👍

iteles commented 7 years ago

Agree with @Danwhy, we know this is going to be growing quite a lot from here on out and we're no longer at a prototype stage.