nestjs / nest

A progressive Node.js framework for building efficient, scalable, and enterprise-grade server-side applications with TypeScript/JavaScript 🚀
https://nestjs.com
MIT License
66.73k stars 7.54k forks source link

DISCUSSION: Routes Alias, Nest Router Module #331

Closed shekohex closed 6 years ago

shekohex commented 6 years ago

This Discussion is to make some decisions to Improve Nest Routring.

As The Title Says , we could enable Multi Routes for the same handler ! Not clear enough ?
I'll make it simpler , we could have 2 end points for the same controller :smiley: Need an Example ? No problem

// here is a 2 end-points for the same controller
@Controller(['user', 'u'])
class UserController {
  @Get(':id')
  findOne(@Param('id') id) {
    return { userId: id }
  }
}

here is another one

@Controller('cats')
class CatsController {
  @Get(['all', '/', 'records'])
  async findAll() {
    // returning all cats ..
  }

  @Get(':id')
  findOne(@Param('id') id) {
    return { catId: id }
  }
 }

How ?

the question now , How we could achieve this ? the answer is pretty easy , because Nest is using Express under the hood , we could just pass an array of routes to the Express router 1 , And it will handle the rest.
Thanks to Express, and we could even do much , we could add regex to the routes :smiley: ( but it is not recommended )

Why !

Since I was Working on Nest Router Module - still under development - it is now using a Dynamic Module 2 rather than need to access to NestContainer but I need to add More to it , as I was discussing with @wbhob .

Here is how it works now
`app.module.ts`

...

import { routes } from './app.routes';

@Module({
    modules: [
        CatsModule,
        DogsModule,
        NinjaModule,
        RouterModule.forRoutes(routes),
    ],
})
export class ApplicationModule { }
`app.routes.ts`

...

export const routes: Routes = [{
    path: '/ninja',
    controller: NinjaController,
    children: [
        {
            path: '/cats',
            controller: CatsController,
        },
        {
            path: '/dogs',
            controller: DogsController,
        },
    ],
}];

you could check this example here

it works as expected.

/ninja -> NinjaController
/ninja/:id -> NinjaController@findOne
/ninja/dogs -> DogsController
/ninja/cats -> CatsController
/ninja/dogs/:id -> DogsController@findOne
/ninja/cats/:id -> CatsController@findOne

but here is the problem , if we try to /ninja/:id/cats it will fail 404 , it makes sense. The RouterModule is just a remapper , it will remap all The controller to it's parents, but except it's methods.

RouterModule can access the Controller Methods an even more , it can Remap it also to the parent Controller

Then What ?

because Nest is using Reflector, the metadata can have only one unique identifier, so that it can't have multi route or path prop for the same handler, but it could be an Array , and this - as I see - will solve the problem.

so I need your opinions about it , feel free to give me your feedback :+1:

Footnotes:
wbhob commented 6 years ago

but here is the problem , if we try to /ninja/:id/cats it will fail 404 , it makes sense.

I mentioned in our conversation on Gitter, this should be the expected behavior.

shekohex commented 6 years ago

yeah, That's Why we are DIscussing this here, the RouterModule is just edit the path metadata for the childs, so It makes sense to that's why it doesn't work . if we use an array that wrap all routes includes the parent route , and point to the same handler , i think it will solve the problem.

kamilmysliwiec commented 6 years ago

Hi @shekohex, Honestly, I don't understand why RouterModule allows setting a path for a controller since it's possible directly using the decorator, and imo, it's a better way. I think RouterModule should solve issues that are not handled by the decorators, thus it shall provide a way to create a hierarchy of paths by tying up a particular path to the module.

shekohex commented 6 years ago

Hi Kamil, I just provide the path property to make some sense when it come to Organize the paths, since the path property is optional, you can omit it , RouterModule will extract the path metadata and use it instead. and about organizing the controllers in hierarchy way , it dose :smiley: , but right now , it will look for one level depth , maybe in future will be more dynamic .

kamilmysliwiec commented 6 years ago

@shekohex yeah, but your solution concerns controllers. Why not use modules instead? For example:

export const routes: Routes = [{
    path: '/ninja',
    module: NinjaModule,
    children: [
        {
            path: '/cats',
            module: CatsModule,
        },
        {
            path: '/dogs',
            module: DogsModule,
        },
    ],
}];

it would allow creating a hierarchy of modules. The easiest way to do that is to:

  1. Inject ModulesContainer in the dynamic module
  2. Go through defined routes
  3. Just override a controller path metadata using
wbhob commented 6 years ago

Kamil does present an interesting structure. I think long-term that makes more sense @shekohex important to note that children should not inherit from their parents, that would get confusing

shekohex commented 6 years ago

Did you see this will make more control for routring ?! to be honest, I don't think this way - route modules over controller - will make more control of what you can do , idk maybe in the future we should consider this structure . BUT until now that's not the problem, as I described in the issue , that we should consider using Array of paths to make the full control of the routes hierarchy.

Here is How

instead of changing the child controller path metadata to the new path which is parent path + his path i will also override it's method path's , we will create an Array of paths that will contain also the parent methods paths and the current child method path after that we can happily change the path metadata with this Array , finally .. let the hard work to Express Router

kamilmysliwiec commented 6 years ago

@shekohex but it doesn't make sense to move controllers from a single module to the different hierarchies. Additionally, your solution allows overriding path entirely that is strange a little. Referring to the multiple paths feature, why do you think we should handle this at the framework-level? Since the business logic should be moved outside from the controllers, isn't it more expressly to create a 2 separated methods?

shekohex commented 6 years ago

@wbhob I know I know , but let's Imagine that I would to make a simple RESTful API for my Blog. and I have 2 Modules UsersModule and ArticlesModule

@Module({
    controllers: [
        UsersController,
     ...
    ],
})
export class UsersModule { }
@Module({
    controllers: [
        ArticlesController,
     ...
    ],
})
export class ArticlesModule { }

and all the logic - you know - CRUD is ok and there is no problem. but here when it come when I want to add some Engagement to my Articles, then I decided to make a simple CommentsController then I updated my ArticlesModule , added the CRUD methods to it , then I just realized that i need to get also the Article id to add New Comment or update it .. since it's related to it. then the URL should be /article/:articleId/comment/:commentId

so should I use this pattern of long url in all CommentController Methods ? the RouteModule should solve this for me. since it will override CommentController methods and add /article/:articleId prefix to all it's method, and also will make it possible to access it without this prefix, I think we could make an option for this too.


shekohex commented 6 years ago

@kamilmysliwiec , yeah that's now makes sense , we should use Modules instead of Controllers. I will think about it again.

kamilmysliwiec commented 6 years ago

duplicate of #255

lock[bot] commented 4 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.