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

[BUG] Children Controller Routes should be declared before parent #2706

Closed
EinfachHans opened this issue May 13, 2024 · 10 comments · Fixed by #2711
Closed

[BUG] Children Controller Routes should be declared before parent #2706

EinfachHans opened this issue May 13, 2024 · 10 comments · Fixed by #2711

Comments

@EinfachHans
Copy link
Contributor

Describe the bug

I have the following setup:

@Controller({
  path: '/parent',
  children: [ChildrenController]
})
export class ParentController {

  @Post(':id')
  public updateUser() { ... }
}
@Controller('/children')
export class ChildrenController {

  @Post()
  public async getAll() { ... }

}

when performing a POST against /parent/children, this is mapped into the post of the ParentController, because children also matches :id.

To Reproduce

See above, let me know if you need a repo

Expected behavior

I think children routes should be always declared before the parent routes, does this make sense or brings other problems i don't see atm? 🤔

Code snippets

No response

Repository URL example

No response

OS

macOS

Node version

20.12.2

Library version

7.69.3

Additional context

No response

@Romakita
Copy link
Collaborator

Hello @EinfachHans
You've right, but changing that is probably a breaking change :(

Let me get a time to test if it's possible.

See you
Romain Lenzotti

@EinfachHans
Copy link
Contributor Author

@Romakita maybe we could make this as a "opt-in" feature first via the configuration, then change the default & deprecate in next major and remove the config again in the next major after that?

@Romakita
Copy link
Collaborator

Romakita commented May 23, 2024

Yes, when can try to do that.

The problem is probably located here:

from(token: TokenProvider, parentMiddlewares: any[] = []) {

children.forEach((token: Type<any>) => {

I'm a bit over staffed to do that. But If somebody had a time to add the opt-in, I'll be happy to review the PR ;)
It doesn't seems to be a huge work to add that :)

See you @EinfachHans

Romain

@EinfachHans
Copy link
Contributor Author

@Romakita Do you have a suggested name for the config parameter?

@Romakita
Copy link
Collaborator

Hum 😅... maybe appendNestedRoutesBefore ?

@EinfachHans
Copy link
Contributor Author

@Romakita just want to start a new PR. I tried to get the tests run in the current production branch, without changed made and get the following error:

@tsed/engines: ✖ ERROR: Error: Cannot find module '/Users/einfachhans/IdeaProjects/tsed/tools/mocha/register' imported from /Users/einfachhans/IdeaProjects/tsed/node_modules/mocha/lib/nodejs/esm-utils.js
@tsed/engines: Did you mean to import "/Users/einfachhans/IdeaProjects/tsed/tools/mocha/register.js"?
@tsed/engines:     at finalizeResolution (node:internal/modules/esm/resolve:265:11)
@tsed/engines:     at moduleResolve (node:internal/modules/esm/resolve:933:10)
@tsed/engines:     at defaultResolve (node:internal/modules/esm/resolve:1157:11)
@tsed/engines:     at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:390:12)
@tsed/engines:     at ModuleLoader.resolve (node:internal/modules/esm/loader:359:25)
@tsed/engines:     at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:234:38)
@tsed/engines:     at ModuleLoader.import (node:internal/modules/esm/loader:322:34)
@tsed/engines:     at defaultImportModuleDynamically (node:internal/modules/esm/utils:197:36)
@tsed/engines:     at importModuleDynamicallyCallback (node:internal/modules/esm/utils:219:12)
@tsed/engines:     at Object.exports.doImport (/Users/einfachhans/IdeaProjects/tsed/node_modules/mocha/lib/nodejs/esm-utils.js:35:34)
@tsed/engines:     at formattedImport (/Users/einfachhans/IdeaProjects/tsed/node_modules/mocha/lib/nodejs/esm-utils.js:9:28)
@tsed/engines:     at exports.requireOrImport (/Users/einfachhans/IdeaProjects/tsed/node_modules/mocha/lib/nodejs/esm-utils.js:42:34)
@tsed/engines:     at exports.handleRequires (/Users/einfachhans/IdeaProjects/tsed/node_modules/mocha/lib/cli/run-helpers.js:94:34)
@tsed/engines:     at async /Users/einfachhans/IdeaProjects/tsed/node_modules/mocha/lib/cli/run.js:349:25 {
@tsed/engines:   code: 'ERR_MODULE_NOT_FOUND',
@tsed/engines:   url: 'file:///Users/einfachhans/IdeaProjects/tsed/tools/mocha/register'
@tsed/engines: }

Any idea why?

@Romakita
Copy link
Collaborator

Just run test for the concerned package ;)

Run entire test on this repo will take along time.

Also have you installed the project using ˋyarn install` ?

See you

@EinfachHans
Copy link
Contributor Author

Yes, only running the platform-router tests works, but anyway it's weird that i can't run the engine tests in a new fresh environment. Of course i installed the dependencies, i followed the contributing guide 🤔

Copy link

🎉 Are you happy?

If you appreciated the support, know that it is free and is carried out on personal time ;)

A support, even a little bit makes a difference for me and continues to bring you answers!

github opencollective

@Romakita
Copy link
Collaborator

🎉 This issue has been resolved in version 7.70.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

2 participants