-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
fix(core): let the middleware can get the params in the global prefix #10390
fix(core): let the middleware can get the params in the global prefix #10390
Conversation
Pull Request Test Coverage Report for Build 88e8b676-a087-4894-ad55-c3b4e77ab62f
💛 - Coveralls |
packages/core/router/interfaces/exclude-route-metadata.interface.ts
Outdated
Show resolved
Hide resolved
Does this pull request fix the issue described in #8844 (comment) by any chance? If not, can we please fix that bug in this PR please and add some tests? Please let me know if that's not possible and I'll create a separate issue and open a PR. EDIT: here's the ticket in case you decide to fix the bug #10566 |
@mareksuscak I think I can get this PR to fix #10566 with a few small tweaks. But I think this should be two different questions. I'm not sure if we need to open another PR. |
|
@mareksuscak Hi, I tried to fix the issue described in #8844 (comment) and added some tests in this PR. I’d appreciate it if you have time to review the commit and give me some advice. |
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.
Thanks for your contribution, @CodyTseng, I haven't had time to look into this issue since last week. I left a few suggestions for you, and I hope you find them useful 😄
Thanks for your useful suggestions, @thiagomini ! I will look carefully at your suggestions and optimize the code later. |
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.
Hey Cody, first, I would like to thank you again for your efforts to contribute to NestJS. Nonetheless, I ask you to review the tests you've created here. I hope my explanation clarified why we should avoid testing private methods, but let me know if you are not sure yet.
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.
This LGTM (Looks good to me) now, Cody. Thanks for considering my reviews 😃
Hey @kamilmysliwiec , could you review this too? |
Apologies for not reviewing this PR sooner.. There's a few conflicts that occurred after we introduced a new PathsExplorer class https://github.com/nestjs/nest/blob/master/packages/core/router/paths-explorer.ts. Would you be able to resolve them @CodyTseng? |
Of course! I will resolve the conflict later. |
087de17
to
a1f17e2
Compare
Hey @kamilmysliwiec, I've resolved conflicts. You can review it when you have time 😊 |
I'm wondering if this PR solves this issue #9990 as well 🤔 |
Unfortunately, this issue #9990 has not been solved in this PR. I'm also following this issue, if I think of a suitable solution, I'd be happy to create a PR for it. I think the following method needs to be modified. nest/packages/core/middleware/utils.ts Lines 94 to 111 in 03efdce
|
Co-authored-by: Kamil Mysliwiec <mail@kamilmysliwiec.com>
LGTM |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #9776, #10566
What is the new behavior?
Does this PR introduce a breaking change?
Other information