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

Global prefix for a module / route tree #255

Closed
lukgri opened this issue Nov 18, 2017 · 41 comments · Fixed by #389
Closed

Global prefix for a module / route tree #255

lukgri opened this issue Nov 18, 2017 · 41 comments · Fixed by #389

Comments

@lukgri
Copy link

lukgri commented Nov 18, 2017

Hi,

there is an older issue, which got implemented: #40 - allowing to
setGlobalPrefix('api')
on the app instance.

This is a very neat feature and since I'm looking to create some sort of a "route tree", it would be great to set some "route extending strategy".

Say I want to create a route tree like this:
-> api (from app.setGlobalPrefix)
--> /users (UsersModule with prefix 'users' for all of it's 'children' modules and controllers)
---> /posts (PostsModule with prefix 'posts' for all of it's 'children' modules and controllers)
----> /comments (Now I could declare a CommentsController with route 'comments' and this controller's route would be composed from all of the ancestors above, in this case: 'api/users/posts/comments'
----> /upvotes (this one would then be 'api/users/posts/upvotes')

that means I'd like to somehow set the 'module route prefix' for all of it's children's modules & controllers.

If there's already a way to achieve this, please point me in the right direction, otherwise it would be pretty nice to have this.

Thanks

@shekohex
Copy link
Contributor

I'm also looking for something like this , It would be awesome to have a Control of Module prefix
or another way to achieve this

@Zeldaze
Copy link

Zeldaze commented Nov 18, 2017

I agree, this will help keep our applications modular. At the moment I must specify my controller base as /api/users, /api/posts, /api/news etc. It's only a small inconvenience for small apps, but a redundancy nevertheless.

@wbhob
Copy link
Contributor

wbhob commented Nov 18, 2017

I think there should be a path property on parent modules, like id in Angular modules.

@gelito
Copy link
Contributor

gelito commented Nov 19, 2017

A reference/example on pure Javascript + Express of how to do it:
http://katieleonard.ca/blog/2016/nested-routes-with-expressjs/

It's quite important with the issue #257 because in some cases you have nested elements:

\book:bookHash\chapter:chatperHash

And you want to test that bookHash exists (into the BookController) and also chapterHash exists (into the ChapterController)

@kamilmysliwiec
Copy link
Member

The issue is that the modules are not always just a tree. Sometimes they look more like a graph, for example, there might be a module, UsersModule, which contains users business logic and the UsersController inside. The module prefix is users. Now few modules may depend on the UsersModule, but the UsersModule should not 'extend' they API prefixes. Irrelevant whether this is a good practice or not, but it's possible.

@gelito
Copy link
Contributor

gelito commented Nov 20, 2017

@kamilmysliwiec I don't know if it's a good or bad practice, but Angular has the ability of nested routes.

It's not obligatory and is easy to have a workaround, of course, but could be a good improvement.

@kamilmysliwiec
Copy link
Member

@gelito nested routes are a good idea, but I can't see a good way to achieve this now. As I said above, module prefix may cause a lot of issues 🙁

@gelito
Copy link
Contributor

gelito commented Nov 21, 2017

Understood!

BTW great job for all!

@lukgri
Copy link
Author

lukgri commented Nov 21, 2017

isn't there any hack or some sort of middleware that would let us do that?

@wbhob
Copy link
Contributor

wbhob commented Nov 21, 2017

What is the possible issue with a module prefix?

@kamilmysliwiec
Copy link
Member

@wbhob i have explained it below ^

@ValentinFunk
Copy link

Would it be possible to separate routing/controllers and things like services? The same way it's done in Angular for example, where you declare where to mount which modules into the routing tree but you can import and use a module in another without using any of it's routes?

@kamilmysliwiec
Copy link
Member

@kamshak hmm.. any ideas about the API? I'm trying to imagine how we could do it well

@shekohex
Copy link
Contributor

shekohex commented Nov 22, 2017

I think this will help
@kamilmysliwiec
https://github.com/angular/angular/blob/5.0.2/packages/router/src/router.ts#L190-L760
good luck 😃
and also take a look add this article , it should give us any idea 👍
https://vsavkin.com/angular-2-router-d9e30599f9ea

@kamilmysliwiec
Copy link
Member

@shekohex haha 😄 well, I know how it's working in Angular, I was asking about the Nest API proposal

@shekohex
Copy link
Contributor

Hahah 😂, sorry for misunderstand
but here is what I'm trying to do right now
I'm here trying to make a nested routes like a Parent route and a childs
the simplest idea get in my mind is to make the ability to add a Parent controller to another one
like this

@Controller('foo') class Foo { }
@Controller('bar', Foo) class Bar { }

then if I checked the Bar class route now it will be foo/bar instead of bar
what I did , is just played with reflect metadata

export function Controller(prefix?: string, parentController?: Controller): ClassDecorator {
    let path = isUndefined(prefix) ? '/' : prefix;
    if(parentController) {
       const parentPath = Reflect.getMetadata(PATH_METADATA, parentController);
       path = parentPath + '/' + path; 
    }
    return (target: object) => {
        Reflect.defineMetadata(PATH_METADATA, path, target);
        Reflect.defineMetadata(PARENT_ROUTE, parentController, target);
    };
}

However, I have it now working , but I think it is not the best solution for this issue
so any ideas ?

@kamilmysliwiec
Copy link
Member

@shekohex honestly, it's the best idea for now heh

@shekohex
Copy link
Contributor

I think it's just a Hack 😄
anyway I'm now thinking how to make the child controller having access to his Parent eg: parms, body..
for example, If I have a 2 Controller PostsController, CommentsController
and what I want to achieve is to have this routes

GET posts/:id
GET posts/:id/comments
@Controller('posts') 
  export class PostsController {
        @Get(':id')
        getPosts(@Param('id') id: number) {
            ...
            //go to database and returing the post
            ...
        }
    }

@Controller('comments', PostsController) 
   export class CommentsController {
        @Get()
        getPostComments() {
            // now I need to get the controle of the post
            // or post Id, to get it's comments
        }
    }

so any Help in this ?
or any New Idea !

@kamilmysliwiec
Copy link
Member

@shekohex

  1. Use middleware instead
  2. Attach post to req and call next()

@ValentinFunk
Copy link

@kamilmysliwiec: Will try to sketch out a quick consumer API. The challenge I see at the moment is that with the more declarative style you might loose some of the ease of the Decorators. (e.g. you'd always have to wire it up somewhere into the routing tree which might be tricky using decorators)

@wbhob
Copy link
Contributor

wbhob commented Nov 23, 2017

This way, more options can be added later without breaking changes:

@Controller('comments', {
     parent: ParentController,
     // OR THIS (preferably make both available)
     parents: [
        // In hierarchical order of importance
        ParentController,
        OtherParentController
    ]
}) 
   export class CommentsController {
        @Get('/child')
        getPostComments() {
            // implementation
        }
    }

In my ideal world, though, the whole decorator metadata would look like this:

@Controller({
    path: '/comments', (force the leading slash)
    components: [
          // If someone wanted to scope them here, like
          // `providers` in Angular
    ],
    parent: ParentController,
    // OR THIS (preferably make both available)
    parents: [
       // In hierarchical order of importance
       ParentController,
       OtherParentController
    ]
}) 
export class CommentsController {
    @Get('/child')
    getPostComments() {
        // implementation
    }
}

@shekohex
Copy link
Contributor

good idea 👍
and here is what I did, I make it easier for creating a something like a RoutesTree .
create a simple array of objects that have all controllers like this

describe('RouterTreeFactory', () => {
    it('should make a router tree', () => {
      @Controller('grandpa') class GrandPa { }
      @Controller('parent') class Parent { }
      @Controller('child') class Child { }

      const routerTree: IRouterTree = [{
        parent: GrandPa,
        childs: [Parent]
      },
      {
        parent: Parent,
        childs: [Child]
      }];

      appConfig.createRouterTree(routerTree);
      const parentPath = Reflect.getMetadata('path', Parent);
      const childPath = Reflect.getMetadata('path', Child);
      expect(parentPath).to.be.eql('grandpa/parent');
      expect(childPath).to.be.eql('grandpa/parent/child');
    });
  });

here is you don't have to edit @Cotroller() anymore just making a simple hierarchical route tree.
now before bootstrapping the application we register the route tree, and let the magic happen 😄

async function bootstrap() {
  const app = await NestFactory.create(ApplicationModule);
  const routerTree = [{
        parent: CatsController,
        childs: [DogController, BirdsController]
      }];
  await app.createRouterTree(routerTree);
  await app.listen(3000);
}

@shekohex
Copy link
Contributor

but now we have a small problem , let's imagine we have the same 2 controllers

...
//simple routes tree
export const routesTree = [{
    parent: CatsController,
    childs: [DogsController]
}];

...
@Controller('cats')
export class CatsController {
  constructor(private readonly catsService: CatsService) {}

  @Get()
  async findAll(): Promise<Cat[]> {
    return this.catsService.findAll();
  }

  @Get(':id')
  //should give me an validation error when I hit `cats/dogs` 
  findOne(@Param('id', new ParseIntPipe()) id) {
    // logic
  }
}
...

@Controller('dogs')
export class DogsController {
    constructor(private readonly dogsService: DogsService) { }

    @Get()
    async findAll() {
        return await this.dogsService.findAll();
    }
}

and It will also throw 404 Not Found - Cannot GET /cats/0/dogs Error if i will try to go to /cats/0/dogs
I think we are getting closer 😄 ..
so any Idea how to solve it !

@kamilmysliwiec
Copy link
Member

I like the proposed approach with passing the parents controllers into the @Controller() decorators - no breaking changes + consistent. The question is, don't you think that it looks a little bit strange? 😄 Maybe I'm the only one who has some claims thus it's better to ask 😅

@shekohex
Copy link
Contributor

shekohex commented Nov 23, 2017

@kamilmysliwiec maybe 😄
anyway I think the problem here not to related to the pattern that how we can make it works, I mean
It now doesn't matter should we make it from @Controller() or a RouteTree

The Problem is the Parent Controller doesn't know that he Has a childs !

I mean by that It , If the Parent Controller Has a 2 Children's , Then the Parent Should HAVE the ability to Remap it's routes to its children's .

@Controller('dogs')
export class DogsController {
    constructor(private readonly dogsService: DogsService) { }

    @Get()
    async findAll() {
        return await this.dogsService.findAll();
    }
}
...
@Controller('cats')
export class CatsController {
  constructor(private readonly catsService: CatsService) {}

  // `/cats/`
  @Get()
 // this should be also remapped to `/cats/dogs` and the target should be `DogsController.findAll`
  async findAll(): Promise<Cat[]> {
    return this.catsService.findAll();
  }
  
  // `/cats/:id/`
  @Get(':id')
  // and this should be also remapped to `/cats/:id/dogs`
  findOne(@Param('id', new ParseIntPipe()) id) {
    // logic
  }
}

and so on..

My Mind will Blow up
image

@cojack
Copy link
Contributor

cojack commented Nov 23, 2017

@shekohex

The Problem is the Parent Controller doesn't know that he Has a childs !

It sounds like a big mistake. Parent's shouldn't know about they childs at all! Like in OOP, please do not reinvent the wheel.

Btw, your example is mysterious, you're talking about Dogs Controller without showing it, can you improve it?

@ValentinFunk
Copy link

ValentinFunk commented Nov 23, 2017

I'm with @shekohex on this, parent should define child - not the other way round (that would be a bit strange, you want to include a module from somewhere and have to modify it just to mount it into your routing tree).

Regarding

Parent's shouldn't know about they childs at all! Like in OOP

Yes, but I'd argue that routes are composition not inheritance. E.g. Animal class does not need to know that there are Dogs, Cats and Dolphins but the Car class absolutely needs to know about it's engine, wheels and fuel!

To build the routing tree separately from the controllers you could do this:

/*
  This controller specifies an explicit route tree. Routes from the child modules
  are mounted into the module's routing tree.
*/
@Module({
  modules: [ UsersModule, AdminModule ],
  controllers: [ HealthCheckController ],
  routes: {
    path: '/',
    // NOTE: In theory we could add more stuff here (e.g. Middleware)
    children: [
      { path: '', routes: HealthCheckController },
      { path: '/users', routes: UsersModule },
      { path: '/admin', routes: AdminModule }
      // This also allows to mount another express app, e.g.
      { path: '/external', mount: express() }
    ]
  }
}) export class AppModule { }

@Controller()
export class HealthCheckController() {
  @Get('/healhtz') returnNothing() { }
}

/*
  Each module, e.g. the Admin Module exports a routing tree, too.
  If you do not speficy a routing configuration the routes are mapped by the annotations.
  If you do specify routes it will always overwrite the component routes though.
*/
@Module({
  controllers: [ DashboardController, AnalyticsController ]
}) export class AdminModule { }

@Controller('/dashboard')
export class DashbaordController {
  @Get('/totalSales') totalSales() {
    return 1230;
  }
}

@Controller('/analytics')
export class AnalyticsController {
  @Get('/users/:id') activeUsers() {
    return 1230;
  }
}

Resulting Tree:

/
  /healthz
  /admin
    /dashboard
      /totalSales
    /analytics
      /users/:id
  /users
    ... user module paths mounted here

The issue i see is that when you're mixing patterns it gets a bit confusing. When you allow to specify things via decorator as well
as a declarative tree - it get's tricky to prioritize. I'd always say explicit beats implicit when deciding which to use. This means if you do routes: [] in your module you simply won't mount any of the child module's controllers into your application (which you might want to do).

I believe this is why e.g. the Angular Router does not use Annotations - but i'm not 100% sure this is the reason. Maybe there is an elegant way to solve this problem.

@shekohex
Copy link
Contributor

@cojack , I think you may miss that part

If the Parent Controller Has a 2 Children's , Then the Parent Should HAVE the ability to Remap it's routes to its children's

I don't know if I can't make It clear to you , But it's just the simple way to make the tests always green, I mean by

the ParentController Should Have a Access to its Childs

That The RouterTreeFactory 👇 should have access to the child's Methods to Remap it to its Parents
but anyway I think it's not the best practice and anti-pattern 😒

...
export interface IRouter {
        parent: Controller,
        childs: Array<Controller>
}
export interface IRouterTree extends Array<IRouter> {}
export interface IRouterTreeFactory {
    buildRouterTree(tree: IRouterTree);
}

...

export class RouterTreeFactory implements IRouterTreeFactory {
    public async buildRouterTree(tree: IRouterTree):  {
            tree.forEach(route => {
                const parentRoute = Reflect.getMetadata(PATH_METADATA, route.parent);
                route.childs.forEach(child => {
                    const childRoute = Reflect.getMetadata(PATH_METADATA, child);
                    let newRoute = parentRoute + '/' + childRoute;
                    Reflect.defineMetadata(PATH_METADATA, newRoute, child);
                });
    
            })
    }
}

btw, I updated the example
#255 (comment)

@shekohex
Copy link
Contributor

Hi @kamshak 😃

The issue i see is that when you're mixing patterns it gets a bit confusing. When you allow to specify things via decorator as well

do You mean when I mention to use @Controller('prefix', Parent) at first then I used another way RoutesTree. am I alright !
Ok , the Point that what I'm love to achieve is something similar to Angular Route Syntax or Pattern
but your example Here seems a good way to make that, but can you explain more ?
An approach or a way to get closer ?
Thank You

@kamilmysliwiec
Copy link
Member

kamilmysliwiec commented Nov 23, 2017

@kamshak I like your proposal, but we have to keep in mind that Nest is not only for the REST API's. In the incoming release, you'll be able to create an application context (without running any HTTP server behind) therefore routes property would be redundant. The same scenario's with microservices.

Based on your suggestion, I think we should consider sth like this:

@Module({
  modules: [ 
    UsersModule, 
    AdminModule,
    RouterModule.configure({
      path: '/',
      children: [
        { path: '/users', routes: UsersModule },
        { path: '/admin', routes: AdminModule },
      ]
    })
  ],
  controllers: [HealthCheckController],
}) 
export class AppModule { }

btw @wbhob it'd resolve the issue with module prefix 🙂

@kamilmysliwiec
Copy link
Member

kamilmysliwiec commented Nov 23, 2017

hmm, this syntax opens few new possibilities:

@Module({
  modules: [ 
    UsersModule, 
    AdminModule,
    InterceptorsModule.use(
      InterceptorX,
      InterceptorY,
    ),
    GuardsModule.use(
      GuardX,
      GuardY
    ),
    MiddlewaresModule.configure(
      {
        middleware: MiddlewareX,
        forRoutes: []
      }
    )
  ],
  controllers: [HealthCheckController],
}) 
export class AppModule { }

= bind module scoped guards & interceptors etc etc

hmm.. but for guards, interceptors, exception filters and pipes this might be redundant (why not to reuse available decorators?)... However, for middlewares & router configuration it should fit great

@wbhob
Copy link
Contributor

wbhob commented Dec 9, 2017

I've opened a PR. This is sort of what we're talking about, but in a more nest-friendly way. My intended usage is like this:

@Module({
    path: '/child-route'
})
export class MyChildModule { }

Essentially, at runtime, it reflects the parent module's paths back onto each controller.

@VinceOPS
Copy link

Hi guys,

I looked into this with a lot of interest. Kamil did not approved the PR, for reasons I can understand.
However, does anyone has a solution to this?

We're about to split our API in 3 modules and it'd be lovely to be able to have a global prefix for all controllers in each module (eg /module1, /module2, /module3...).
Thanks!

@br0wn
Copy link

br0wn commented Jan 26, 2018

@VinceOPS This is how we've done it so far

async function bootstrap() {
    const server = express();
    const config = require('../../etc/config.js');

    const apiFactory = new NestFactoryStatic();
    const api = await apiFactory.create(ApiModule, server);
    api.setGlobalPrefix("/api/v1");
    await api.init();

    const adminFactory = new NestFactoryStatic();
    const admin = await adminFactory.create(AdminModule, server);
    admin.setGlobalPrefix("/admin");
    await admin.init();

    http.createServer(server).listen(config.server.port);
}

bootstrap();

@chanlito
Copy link

@VinceOPS looks good, It is the proper way?

@sagic
Copy link

sagic commented Jan 30, 2018

@br0wn
perfect solution for me!
thanks

@VinceOPS
Copy link

@br0wn hey! Thanks for sharing your approach. I must consider it, but it looks nice.
@kamilmysliwiec Do you see any downsides in br0wn's approach?

Thanks guys!

@shekohex
Copy link
Contributor

Actually @wbhob proposed a very good solution for this , but it rejected.
Me I was Developing a Module -Nest RouterModule-
to organize your Routes, but there is no luck without provide some core API
Right now I have a local repo of Nest , it has feature that every Module could have a path property that path will be a prefix for all Controllers in this Module , another good thing if i imported that Module in another one , it will be a child of it and again all Controllers in this child modules will be prefixed by { parent module prefix + this module prefix }
So if I have a UsersModule and OffersModule .
the OffersModule has one Controller UserOffers and that Module has a prefix path = 'offers';
So all the Controllers in this Modules has a prefix for all routes.
On the other side , The UserModule my have another Controller , also have a prefix path = 'user';
And the interesting part is If I Imported OffersModule in UserModule as in imports[].
The Controllers in the OffersModule will be prefixed by user/offers.
And it works pretty well.
I could make another PR , but let me see if This still interesting to you , please vote if you need this in Nest.

And Thanks also to Wilson for this great idea

@wbhob
Copy link
Contributor

wbhob commented Jan 30, 2018

@shekohex #297

@shekohex
Copy link
Contributor

continue with this PR #389

@lock
Copy link

lock bot commented Sep 25, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.