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(RouterModule): add reolvePath method to get controller full path. #32

Merged
merged 2 commits into from
Nov 4, 2018

Conversation

shekohex
Copy link
Member

@shekohex shekohex commented Oct 14, 2018

As @marsprince mentioned in his PR #31 that nestjs dosen't resolve or take into account MODULE_PATH metadata when it is coming to resolve Controller path in Middlewear resolver.
@marsprince introduced that we could have controller proprietary at Route interface that we could then override the PATH_METADATA metadata value to the new modulePath + controllerPath in which as he said: ..also make us lose ability to define own route in Decorator @Controller .. and I really appreciate his hard work, but IMO that is not a good solution to mutate the metadata that i think will cues some unpredictable issues as MUTATING OTHER'S DATA IS NEVER BEEN A GOOD SOLUTION.

so that i introduced a new fancy method RouterModule#resolvePath that will resolve the full path of any controller so instead of doing so:

consumer.apply(someMiddleware).forRoutes(SomeController);

you should do

consumer.apply(someMiddleware).forRoutes(RouterModule.resolvePath(SomeController));

and that open us a new ideas like implementing something like as others discussing here nestjs/nest#1030 .

at that end thanks again to @marsprince 💙 .

@coveralls
Copy link

Pull Request Test Coverage Report for Build 102

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 100: 0.0%
Covered Lines: 22
Relevant Lines: 22

💛 - Coveralls

1 similar comment
@coveralls
Copy link

coveralls commented Oct 14, 2018

Pull Request Test Coverage Report for Build 102

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 100: 0.0%
Covered Lines: 22
Relevant Lines: 22

💛 - Coveralls

@marsprince
Copy link

👍

@marsprince
Copy link

any plan for new version? @shekohex

@shekohex
Copy link
Member Author

any plan for new version? @shekohex

I'm planning to land this Update by this weekend, maybe in Sun 18 .

@shekohex
Copy link
Member Author

I will add docs for that method in readme and also for the changelog.
And that will land soon

@shekohex shekohex merged commit e8dffc8 into master Nov 4, 2018
@shekohex shekohex deleted the resolve-path branch November 19, 2018 14:29
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.

4 participants