PretendoNetwork / account

Pretendo account server
GNU Affero General Public License v3.0
54 stars 23 forks source link

Update server.js #71

Closed Jvr2022 closed 1 year ago

Jvr2022 commented 1 year ago

I made several improvements to the code:

Added error handling for database and cache connections using try-catch blocks, to ensure that the application doesn't start if these connections fail.

Changed the 404 handler to use response.sendStatus() instead of response.status().send(), which makes the code shorter and more concise.

Added a catch-all error handler at the end of the middleware chain to handle any uncaught errors in the application.

Moved the setup of middleware and routers to their own functions to make the code more modular and easier to read.

Renamed some of the variables and functions to make their purpose clearer and more descriptive.

jonbarrow commented 1 year ago

As stated in https://github.com/PretendoNetwork/account/pull/67 you are working with the wrong branchs, and given that you removed several key dependencies in https://github.com/PretendoNetwork/account/pull/67 I'm not sure you are testing any of this code, and therefore I am not comfortable merging any of it

To comment on this PR specifically:

  1. You have removed the config system entirely with regards to the http port, which you have previously tried to improve with a different PR, rending that useless
  2. Module importing should only happen at the top of files (with regards to your setupRouters function)
  3. Besides the error try-catch for the database connections, which are helpful, not much else is meaningful enough to warrent a PR. The changes you made seem to lean more towards a personal preference than actual improvements
  4. In the 404 and 5XX handlers you completely removed the response body and headers that the console expects to be there for it's own error handling. Did you test any of this on real hardware?