Skip to content

Error handling#71

Open
ezehlivinus wants to merge 28 commits into
anomic30:mainfrom
ezehlivinus:error-handling-#66
Open

Error handling#71
ezehlivinus wants to merge 28 commits into
anomic30:mainfrom
ezehlivinus:error-handling-#66

Conversation

@ezehlivinus

Copy link
Copy Markdown
Contributor

Description

This handled asynchronous errors and other global errors. Try...catch can be omitted. Also, logging errors to files were also implemented. When some unknown error occurs we can keep records of it on logs/*.log files.
The packages, winston was used for logging errors to files, and express-async-errors was used to handle asynchronous errors.

Fixes #66 (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Chore: error handling chore

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@ezehlivinus

Copy link
Copy Markdown
Contributor Author

routes/user.js was used to test the changes.

If this is a good candidate to be merged, then we can update all other files, by removing try...catch blocks.

@ezehlivinus

Copy link
Copy Markdown
Contributor Author

With respect to error logging. One can either do this

//const AppError = require AppError from config/ folder
...
throw new AppError("Missing required fields" , 400);

OR

throw new Error("Missing required fields");

This works, it would be encouraged to use the express response object, if available, to return error responses.
This is because any exception or error thrown using Error or its subclasses would be logged to the log files. It is better to do this:

return res.status().send({
  success: false,
  message: 'invalid password or email'
})

Instead of this:

throw new AppError( 'invalid password or email' , 400);
// OR
throw new Error( 'invalid password or email');

This way, we don't get to log unnecessary errors to the file. Yes, `invalid password or email' shouldn't be logged it should be told to the client/user.

Note: that this commit handles Unknown exceptions or a known one if thrown as described above.

@anomic30

@ezehlivinus

Copy link
Copy Markdown
Contributor Author

With respect to error logging. One can either do this

//const AppError = require AppError from config/ folder
...
throw new AppError("Missing required fields" , 400);

OR

throw new Error("Missing required fields");

This works, it would be encouraged to use the express response object, if available, to return error responses. This is because any exception or error thrown using Error or its subclasses would be logged to the log files. It is better to do this:

return res.status().send({
  success: false,
  message: 'invalid password or email'
})

Instead of this:

throw new AppError( 'invalid password or email' , 400);
// OR
throw new Error( 'invalid password or email');

This way, we don't get to log unnecessary errors to the file. Yes, `invalid password or email' shouldn't be logged it should be told to the client/user.

Note: that this commit handles Unknown exceptions or a known one if thrown as described above.

@anomic30

In the future, this can be solved....but for now, this is very handy. Yes, you throw errors where do not have access to the express response object and return errors where you do.

@anomic30

anomic30 commented Oct 6, 2022

Copy link
Copy Markdown
Owner

Hi @ezehlivinus, we don't need to log unnecessary errors related to login/register.
We should return the error to the client like this:

return res.status(401).send({
  success: false,
  message: 'invalid password or email'
})

Also, we should be able to send errors to the client when the server is down.

Comment thread server/config/appError.config.js Outdated
class AppError extends Error {
constructor(message, statusCode = 500) {
super(message);
this.name = 'Stors';

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be "Storz" instead of "Stors"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.

@ezehlivinus

Copy link
Copy Markdown
Contributor Author

This Pull Request is due for review and merge.

cc/
@anomic30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix Error Handlers and handling

3 participants