Nest: Remove default status code nestjs

[ToonvanStrijp]

I'm submitting a...

[x] Regression 
[ ] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

Currently nest is setting the default status code (201 or 200) up front but this becomes a problem when using the res.redirect since nestjs changes the status code the browsers received a status 200 with a get response even though we did res.redirect.

This is only a problem in fastify since express is taking a status code inside it's redirect method (if not defined express changes the status to 302). res.status(200, '/login')

Expected behavior

I would expect the default behavior of the fastify library. There is a way to solve this issue by doing this: res.status(302).redirect('/auth/login').

The easiest solution that would be possible if for nest to drop their default status. This leaves the default behavior of each library (fastify or express) intact.

What is the motivation / use case for changing the behavior?

As I said I think it's best to have the default behavior of each library (fastify or express) this because users are used to those things.

Environment

Nest version: 6.1.0


For Tooling issues:
- Node version: v10.15.1
- Platform:  Linux

Others:

[kamilmysliwiec]

Currently nest is setting the default status code (201 or 200) up front but this becomes a problem when using the res.redirect since nestjs changes the status code the browsers received a status 200 with a get response even though we did res.redirect.

So it actually became a problem after this latest PR with statuses ;)

The easiest solution that would be possible if for nest to drop their default status. This leaves the default behavior of each library (fastify or express) intact.

We can't do it. Express would return 200 for POST requests by default, which is a breaking change.

res.status(302).redirect('/auth/login')

I don't think that this looks very complicated honestly.

[ToonvanStrijp]

Express indeed returns a default 200 on POST. But when red.redirect() is used express changes the status code to 302 except if a status code is given within the function like this: res.redirect(404, “/not-found”).

So in express this doesn’t form any unexpected behavior cause the redirect function sets the correct status code for you.

In fastify this is different. And normally this wouldn’t be a problem because normally you set a status code where you return the response. But because nest is changing the default behavior it leads to unexpected errors.

If you wanna redirect to another page in fasitfy you would normally just use res.redirect Without setting the status code tot 302 first. Because if the status code is not set fastify will change it to 302.

So in my opinion the default behavior nest applies is in conflict with the default expected behavior of the platform (fastify).

[kamilmysliwiec]

I still don't think that forcing people to write:

res.status(302).redirect('/auth/login').

only when fastify is used is worth introducing a breaking change that will affect every existing application.

[ToonvanStrijp]

But right now it already affects every existing application. (So it’s already kind of a breaking change) Because before 6.1.0 res.redirect without setting the status code to 302 manually worked as expected. But because nest is now setting the status code to a default 200/201 at start of the response this breaks the browsers redirect. (If the browser get a redirect with a status code 200 it doesn’t redirect).

[kamilmysliwiec]

What I think that we can eventually do, is that we can disable setting 200 but still detects the request method and when it's POST, then explicitly set 201. That's the only solution that we can do without breaking the current behavior of existing apps.

Btw, shouldn't fastify explicitly set status code on redirect() call? It should be a platform responsibility.

[ToonvanStrijp]

@kamilmysliwiec we could do that but this also feels a bit weird. It’s indeed fastify’s responsibility. I’ll create an issue their and keep you updated.

[martin-juul]

Would it be possible to add a note to the fastify section in the docs?

[kamilmysliwiec]

Let's track this here https://github.com/nestjs/docs.nestjs.com/issues/463

[johnbiundo]

Docs have been updated. Thanks @ToonvanStrijp

[ToonvanStrijp]

@johnbiundo we could provide this piece of code in the docs to show how to keep the default behavior.

fastifyAdapter.getInstance().decorateReply('redirect', function(status, url) {
    const code = 302;
    if (typeof status === 'string') {
      url = status;
    }
    return this.header('location', url)
      .code(code)
      .send();
  });

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.