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

feat(@nestjs/core): Add RouterModule to the core #3489

Closed
wants to merge 4 commits into from
Closed

feat(@nestjs/core): Add RouterModule to the core #3489

wants to merge 4 commits into from

Conversation

shekohex
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

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

What is the current behavior?

Issue Number: #255 , #389 and #1438

What is the new behavior?

For a long time ago (almost 2 years) I created and maintained the nest-router package as a 3rd party package.
Over that time, we gained a lot of feedback and fixed a lot of issues (especially integration with other packages like @nestjs/swagger for example).

And now, we think it's time to add this package as a module into the nestjs core, and Hopefully it would be useful in a lot of use cases, for example:

  1. API Versioning.
  2. Arrange Routes/Modules hierarchically.

...and the list goes on.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Comment on lines 92 to 96
if (prefix) {
path = prefix + this.validateRoutePath(path);
// Override the metadata with the new path
Reflect.defineMetadata(PATH_METADATA, path, metatype);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think doing this would solve all integration issues with any other packages that depend on routes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for example the swagger integration issues and also this issue nestjsx/nest-router#5

Copy link
Member

Choose a reason for hiding this comment

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

Very uncommon but still: what if I have the same controller used in 2 modules? In this case, I'd override the metadata twice.

@coveralls
Copy link

coveralls commented Nov 28, 2019

Pull Request Test Coverage Report for Build 16552

  • 43 of 43 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 95.316%

Totals Coverage Status
Change from base Build 8083: 0.04%
Covered Lines: 4456
Relevant Lines: 4675

💛 - Coveralls

@tonivj5
Copy link

tonivj5 commented Jan 14, 2020

Any news? 👍

@kamilmysliwiec
Copy link
Member

Let me know if there's anything we can do to help @shekohex!

@shekohex
Copy link
Contributor Author

Hi @kamilmysliwiec
I'm sorry, I was so busy in the past few days, but as I got some time, I'll continue working on this PR again.

The issue here with Middlewares as stated here nestjsx/nest-router#5.
I made a Workaround here so far so good, but I think if we are gonna make this a Core Module, why do we need these workarounds.
So any suggestions to solve this issue? without doing this which I think it does not even work

@kamilmysliwiec
Copy link
Member

Actually, this doesn't look that bad ;)

@tonivj5
Copy link

tonivj5 commented Mar 2, 2020

Is this PR going to be merged as part of 7.0.0 release? 🚀

@shekohex
Copy link
Contributor Author

shekohex commented Mar 3, 2020

Hi @kamilmysliwiec
Any Help here with this Middleware error? I don't know how that happened

@tonivj5
Copy link

tonivj5 commented Aug 3, 2020

Hey, any progress on this? 👀

@UvealSnow
Copy link

Would love to have this feature!

@vahidvdn
Copy link

vahidvdn commented Dec 4, 2020

Any news?

@shekohex
Copy link
Contributor Author

shekohex commented Dec 4, 2020

Hey all, anyone is interested to take over this PR?
As I don't have any free time to do it these days, things gone crazy this year!

@vahidvdn
Copy link

@shekohex @kamilmysliwiec In Angular, I used to pass the custom data object where I define the route (in router module). Then in guard, I can have access to that data. (e.g. isPublic = true)

But here (as described) we should define a custom decorator and decorate the controller's method with it, then in guard, we can have access to the decorator's value (to check whether the API is decorated with that decorator or not)
I think the Angular approach is much better from the maintainability point of view which can be possible with this RouterModule.

@kamilmysliwiec
Copy link
Member

@shekohex I've cherry-picked your changes and created a new PR #6035 let's track this there

@vahidvdn
Copy link

vahidvdn commented Dec 28, 2020

@kamilmysliwiec Would you please consider this for versioning? (Actually the discussion started from here)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants