Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The second argument "Error" overwrites the first argument "status" #58

Closed
Nicolas-yonedA opened this issue Jan 30, 2019 · 2 comments
Closed

Comments

@Nicolas-yonedA
Copy link

I am sorry if I had written rude things because I am poor at English.

When I specified Error as the second argument, there was a key like status or statusCode, so the status code of the first argument was overwritten.

Error is like this.

{ MessageRejected: Email address is not verified.....
    message: 'Rejected',
    code: 'ClientError',
    statusCode: 400}
    

And, I written the following code.

try {
    ....
} catch (e) {
    throw createError(500, e, { errors: [{ msg: 'sending mail was failed' }] })
}

In the case of the above error, the first argument is ignored and the return value of status is forced to 400.
And I return the status of the Error to the client, as the status code of the response.

This is caused by the following code.

  for (var i = 0; i < arguments.length; i++) {
    var arg = arguments[i]
    if (arg instanceof Error) {
      err = arg
      status = err.status || err.statusCode || status

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I think it's strange that status was changed by the first argument , but it will changed again.

@dougwilson
Copy link
Contributor

So there seems to be a lot of opinions on this and we are trying to change this as people want. For example there is an open PR #37 that goes in the opposite direction you want, using the status from even non Error objects.

So I'm not sure what the right answer here is, as it seems like everyone wants it to work a different way. The way it is now was also a pull request to make it take the error from the given Error object and ignore the number argument.

Because of this, I don't really want to change the behavior as there will certainly be folks who arrive saying the behavior you want here is unexpected/wrong too (this actually happened and is why we have the behavior we have now).

Do you have a solution that will satisfy all these requests vs just keep changing the behavior with each request? That would be awesome.

@Nicolas-yonedA
Copy link
Author

Thank you very much for your reply.

I understood that the phenomenon is not a bug but the will of the project.

I am using Express.
The following code exists in app.js of Express.

next (createError (404));
.
.
.
res.status (err.status || 500);

I was blind to this code. And now, I am going to change this to make the source code work as I want.
That is, I will create a module in my project. This module listens to the status code as the first argument.The module executes createError. After that, this module sets the status code of the first argument to the created error with the key, like responseStatus.
When returning an error response with express, I can resolve the problem by first referring to this value as Http Status Code.

Do you have a solution that will satisfy all these requests vs just keep changing the behavior with each request? That would be awesome.

I am sorry, but I do not have enough knowledge to answer clearly.
Actually I felt it was owesome when I read the http-errors source code.
Because there was not much restriction in the order of the arguments.
(I could not find such a description in the document)
So I set an error as the first argument and set the status code as the second argument, the module did what I wanted.
like this.


try {
  throw new createError(400)
} catch (e) {
  createError(e, 500)
  console.log(e)
}

====== output 

{
  .
  .
  .
  status: 500
}

However, I found the following code and I was worried that it might be dangerous.

deprecate('non-first-argument status code; replace with createError(' + arg + ', ...)')

And I decided not to adopt that idea ...

If PR #37 is included in http-errors, I can change status even if I put an error in the second argument.
I expect that it will be able to answer the opinions of many users.

Thank you =)

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

No branches or pull requests

2 participants