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

default response status code should be set at the end #1326

Open
dolsup opened this issue Apr 24, 2019 · 6 comments
Open

default response status code should be set at the end #1326

dolsup opened this issue Apr 24, 2019 · 6 comments

Comments

@dolsup
Copy link

dolsup commented Apr 24, 2019

My suggestion:
Default response status code should be set at the end of response, not before route handlers run.

In my case, I'm trying to catch custom error object(Boom) thrown from handlers and set response status by error type at middlewares. But I always get default 404 unless res.body is set, and I don't know whether the 404 is set by default or my handlers or middlewares.

a known workaround does not work now.

@fl0w
Copy link
Contributor

fl0w commented Apr 25, 2019

You can make a PR if you want, but I'm afraid this might heavily break applications relying on current behaviour.

@dolsup
Copy link
Author

dolsup commented Apr 26, 2019

It can be implemented as a form of app-wide option for compatibility. but I'm not sure if it can be approved because there is no app-wide option in Koa for now.

And I found out a workaround can solve my case by just adding a middleware at the first which is set status to undefined.. so this issue is not urgent but IMHO it should be default behavior in the next major version.

@turbobuilt
Copy link

This doesn't work anymore because of this code in response.js

    assert(Number.isInteger(code), 'status code must be a number');
    assert(code >= 100 && code <= 999, `invalid status code: ${code}`);

@dolsup
Copy link
Author

dolsup commented Jul 22, 2019

@turbobuilt
Oh sorry. I found that I did it with a different method from I described above. In short, I just make every 404 error using Boom so I could distinguish 404 which I throw from default 404. I deal with it in middleware.

I think we should be able to distinguish whether status is default at least for now. This is quite important issue. I'll try to make PR soon.

@turbobuilt
Copy link

turbobuilt commented Aug 13, 2019

Ah, good idea. I think that it is very important as well for UX, because it really frustrated me getting 404s when I was starting simply because I didn't return any data.

@LeonnardoVerol
Copy link

LeonnardoVerol commented Jul 7, 2020

I stumbled upon this issue yesterday.
While I hope it gets fixed, that was my workaround:
(I changed the example a little for this issue)

some-file:

// ...
context.throw([code], [message], { statusCode: [code] });
// ...

on error:

app.on('error', (error, context) => {
        // ...
	if (error.statusCode >= 500) {
	// ...
        }
        // ...
});

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

4 participants