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

feature(redirect decorator) add redirect decorator #2632

Merged

Conversation

johnbiundo
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

No docs yet

PR Type

What kind of change does this PR introduce?

Exposes @Redirect() decorator

@Redirect(url: string, statusCode?: number)

Redirects to the specified url
If no statusCode, sends TEMPORARY_REDIRECT (307)

Method can return an object like
{ statusCode: 302, url: 'http://nestjs.com' }

which will override the decorator parameters.


[ ] Bugfix
[X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:


## What is the current behavior?
<!-- Please describe the current behavior that you are modifying, or link to a relevant issue. -->

Issue Number: N/A


## What is the new behavior?


## Does this PR introduce a breaking change?

[ ] Yes
[X] No


<!-- If this PR contains a breaking change, please describe the impact and migration path for existing applications below. -->


## Other information

Need help with two failing unit tests:
  1. RouterExecutionContext createHandleResponseFn when "renderTemplate" is defined should call "res.render()" with expected args:
  2. RouterExecutionContext createHandleResponseFn when "renderTemplate" is undefined should not call "res.render()":

@kamilmysliwiec
Copy link
Member

Hey @johnbiundo! Thanks for the contribution. Could you somehow get rid of formatting changes? (Maybe run npm run format and npm run lint?)

@johnbiundo
Copy link
Member Author

@kamilmysliwiec fixed the formatting changes (I hope! 😄 )

@johnbiundo johnbiundo force-pushed the feature-add-redirect-decorator branch from de35459 to efa434a Compare July 26, 2019 00:43
@coveralls
Copy link

coveralls commented Jul 26, 2019

Pull Request Test Coverage Report for Build 4136

  • 17 of 17 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 95.249%

Totals Coverage Status
Change from base Build 4098: 0.01%
Covered Lines: 3649
Relevant Lines: 3831

💛 - Coveralls

@@ -49,6 +49,11 @@ export class FastifyAdapter extends AbstractHttpAdapter {
return response.view(view, options);
}

public redirect(response: any, statusCode: number, url: string) {
const code = statusCode ? statusCode : HttpStatus.TEMPORARY_REDIRECT;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we presume that TEMPORARY_REDIRECT should be the default one?

Copy link
Member Author

@johnbiundo johnbiundo Jul 29, 2019

Choose a reason for hiding this comment

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

@kamilmysliwiec Good catch. Both Express and Fastify default to 302 (FOUND). I always associated 302 with TEMPORARY_REDIRECT but to send a 302, it should be FOUND. Make sense?

Copy link
Member

Choose a reason for hiding this comment

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

Make sense now :)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I will switch to 302 (FOUND) and resubmit.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kamilmysliwiec Switched to 302 found, as discussed.

@johnbiundo johnbiundo force-pushed the feature-add-redirect-decorator branch from efa434a to f4d3c03 Compare August 20, 2019 19:47
@kamilmysliwiec kamilmysliwiec added this to the 6.6.0 milestone Aug 24, 2019
@kamilmysliwiec kamilmysliwiec changed the base branch from master to 6.6.0 August 24, 2019 17:55
@kamilmysliwiec kamilmysliwiec merged commit f4d3c03 into nestjs:6.6.0 Aug 26, 2019
@kamilmysliwiec
Copy link
Member

Thanks @johnbiundo! I pushed small performance improvements & merged PR to 6.6.0 branch. Could you create a PR for docs to cover this feature :)?

@johnbiundo
Copy link
Member Author

@kamilmysliwiec Will do!

@johnbiundo
Copy link
Member Author

@kamilmysliwiec Docs PR submitted.

@johnbiundo johnbiundo deleted the feature-add-redirect-decorator branch August 26, 2019 18:51
@lock
Copy link

lock bot commented Nov 24, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Nov 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants