-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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(common) add @WithAlias route decorator #5117
base: master
Are you sure you want to change the base?
Conversation
Add @WithAlias decorator which allows arbitrary aliases to be attached to Controller methods and later retrieved in views and resolved to full route path. Resolves nestjs#3743
@@ -164,6 +167,11 @@ export class RouterExecutionContext { | |||
}; | |||
} | |||
|
|||
public bindResponseLocals(response) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment in PR. This feels unlikely to stay, but if kept—need some guidance on proper typing.
Pull Request Test Coverage Report for Build 0cfed26a-e063-488c-9864-689cb67731c7
💛 - Coveralls |
@@ -0,0 +1,54 @@ | |||
import { join } from 'path'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently, didn't see examples in integration tests that rendered html using the @Render
decorator unless I'm mistaken. So, this tests that along with new changes..
Great job! Can you create a PR to the docs as well? https://docs.nestjs.com/techniques/mvc |
Documents the @WithAlias() decorator and getUrl helper introduced in PR nestjs/nest#5117
Documents the @WithAlias() decorator and getUrl helper introduced in PR nestjs/nest#5117
@kamilmysliwiec , sure thing. Here's a PR for the update to docs: nestjs/docs.nestjs.com#1400. |
Thanks @sjones6! Ill review as soon as possible |
Apologies for the delay! @sjones6 would you like to jump in once again and add an e2e test for fastify too (+ update an example)? This should be the last thing we need in order to get it merged :) |
@kamilmysliwiec , sure—I'll get those in an ping once it's updated. (Looks like some conflicts to resolve too.) |
} | ||
public register(alias: string | Symbol, basePath: string, path: string[]) { | ||
if (this.aliasMap.has(alias)) { | ||
throw new Error(`Conflict ${alias} already registered`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to suggest a new Error object here, like RouterAliasError or something else, Error it too much generic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RouteNameError
private stripSlashes(str: string) { | ||
return str.replace(/^\/?(.*)\/?$/, '$1') | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at the end of the files, there should be a new line
@@ -147,6 +147,7 @@ | |||
"mysql": "2.18.1", | |||
"nats": "1.4.9", | |||
"nodemon": "2.0.4", | |||
"nunjucks": "^3.2.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason to put that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's for a new integration test. As it's in devDeps
it should be fine
Tbh |
Nobody care? |
@cojack: From my point of view RouteName will be the better name and i really like to see this feature in nestjs. |
Any steps to merge pr? |
Hello @cojack @sjones6 @kamilmysliwiec and others, I agree with cojack, @RouteName is a better name which is more releavant than @WithAlias. |
I'm sorry but will this get merged? It seems like thing it's waiting on is name change |
@sjones6 Thank you very much for the code.... @kamilmysliwiec @cojack Any updates soon? :) |
Any updates on this? Seems like too good of a feature to not have as part of the official release. |
yes please. this would be very useful for migrating from symfony |
What is missing for this to be merged? |
Is this feature ever going to be added? |
is this pr going to get merged after almost 2 years? |
@kamilmysliwiec @sjones6, It is a really nice feature, Is this PR going to be merged? |
Any news on this? |
Add
@WithAlias
decorator which allows arbitrary aliases to be attached to Controller methods and later retrieved in views and resolved to full route path.Resolves #3743.
I'm not fully convinced of the implementation in
router-execution-context.ts
especially. This doesn't seem the best way to inject thegetUrl
helper into render context, so open to suggestions. This could be attached toexpressApp.locals
to the same effect inexpress
.Also, open to suggestions on
getUrl
method name if something else is preferred.Lastly, does this require
fastify
support? If so, may require an additional e2e test.PR Checklist
Please check if your PR fulfills the following requirements:
Docs PR: nestjs/docs.nestjs.com#1400
PR Type
What kind of change does this PR introduce?
What is the current behavior?
New feature/not-implemented.
Issue Number: #3743
What is the new behavior?
A new (non-breaking) decorator is available in
@nestjs/common
which allows arbitrary aliases to be attached to Controller route handlers, which can then be resolved to the full path of the route.Additionally, a
getUrl(alias: string, params?: object): string
method is exposed for MVC views / server-rendered template views.Example Controller:
In your views...
Does this PR introduce a breaking change?
Please double check—initial implementation decorates
response.locals
with agetUrl
method. This shouldn't cause breakages and should only be additive.Other information
Some tests and an example were switched from
hbs
template engine tonunjucks
, due to a limitation inhbs
which disallows function evaluation in templates. Based on this issue, this seems highly unlikely to change at any time. Nunjucks, on the other hand, evaluates these as you would expect.