tsed icon indicating copy to clipboard operation
tsed copied to clipboard

[BUG] Children Controller Routes should be declared before parent

Open EinfachHans opened this issue 1 year ago • 1 comments

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

EinfachHans avatar May 13 '24 14:05 EinfachHans

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

Romakita avatar May 13 '24 15:05 Romakita

@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?

EinfachHans avatar May 23 '24 12:05 EinfachHans

Yes, when can try to do that.

The problem is probably located here: https://github.com/tsedio/tsed/blob/980ba21207f29e5e1f57d66240e59f65c0ecef9d/packages/platform/platform-router/src/domain/PlatformRouters.ts#L63

https://github.com/tsedio/tsed/blob/980ba21207f29e5e1f57d66240e59f65c0ecef9d/packages/platform/platform-router/src/domain/PlatformRouters.ts#L117

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

Romakita avatar May 23 '24 12:05 Romakita

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

EinfachHans avatar May 23 '24 13:05 EinfachHans

Hum 😅... maybe appendNestedRoutesBefore ?

Romakita avatar May 23 '24 15:05 Romakita

@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?

EinfachHans avatar May 23 '24 20:05 EinfachHans

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

Romakita avatar May 23 '24 20:05 Romakita

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 🤔

EinfachHans avatar May 24 '24 06:05 EinfachHans

🎉 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

github-actions[bot] avatar May 27 '24 09:05 github-actions[bot]

:tada: This issue has been resolved in version 7.70.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

Romakita avatar May 27 '24 09:05 Romakita