nestjsx / nest-router

Router Module For Nestjs Framework 🚦 🚀
MIT License
619 stars 36 forks source link

Nested routes do not keep prefix #21

Open j-maas opened 6 years ago

j-maas commented 6 years ago

I'm having a bit of trouble keeping my route definitions modular.

In this repro repo I have the AppModule that says hello when /prefix/hello is called. But I intended the SubModule to respond when /prefix/sub/hello is called. But if I just use @Controller('sub') in the SubController, then it responds only to /sub/hello.

Now, I have read a bit of the original discussion with kamilmysliwiec and understand that modules are not meant to map directly to the paths. But right now it feels like needing to declare all routes in a single module hurts modularity.

Is there a way to prefix all controllers of a module as well as their imported controllers?

j-maas commented 6 years ago

I just figured out that an alternative workaround is to repeat the prefix in the AppModule where SubModule is imported:

@Module({
  imports: [
    RouterModule.forRoutes([
      { path: '/prefix', module: SubModule },
    ]),
    SubModule,
  ],
  controllers: [AppController],
  providers: [AppService],
})
export class AppModule {}

Obviously this isn't desirable either, because of the duplication.

shekohex commented 6 years ago

Hi @Y0hy0h , unfortunately that's not an option right now, it's one of Nest design gools, the isolations.

when i was playing with nest, i have a fork of it, actually i achieved something similar to this https://github.com/nestjs/nest/issues/255#issuecomment-361568409

but there is something you may not have noticed, the children prop in Routes could be also an array of modules. so instead of doing something like this

// root.module.ts
...
RouterModule.forRoutes([
      { path: '/prefix', module: AppModule },
      { path: '/prefix', module: SubModule },
    ]),
...

you could just

// root.module.ts

RouterModule.forRoutes([ { path: '/prefix', children: [AppModule, SubModule] } ]),

i know, that's not what you are looking for, but, well.. hmmm... it will work as expected.

j-maas commented 6 years ago

That tip is still useful, thanks! Reads slightly better.

I think for now I can manage without this. I just thought that it would work this way. Is there no way for the RouterModule to prefix the imports of the modules it is configured for?

shekohex commented 6 years ago

i think there is one way to do this, using the ModulesContainer which has no docs yet with that i can access to nest container itself, then from that, i can inspect the imports of any module, and if there is any, i just apply the prefix on it directly, maybe that will done in a dev branch, or maybe in the next major release, because i think this will cause a breaking changes.

but that's not the problem, its about the design goal, maybe we could just open an issue in @nestjs/nest to see what is people will think about that.

i will keep this issue opened until i can get this working.

Regards.

shekohex commented 6 years ago

Hi @Y0hy0h , unfortunately after thinking about that i just came to conclusion, for some reasons we can't do it this way .

let me explain :

1. Breaking Changes:

this will make a very big breaking changes, it is obvious.

2. Unpredictable results:

yes, it is another problem, this was tricky to get in, just imagine that we have four modules, RootModule and the others ModuleA, ModuleB and ModuleC. and we have this module tree:

RouterModule.forRoutes([
      { path: '/a', module: ModuleA },
      { path: '/c', module: ModuleC },
    ]),
and now let's say that `ModuleB` is imported in `ModuleA`'s imports,
and also in `ModuleC`'s imports.
so the `prefix` for `ModuleB` would be `/a/b` or `/c/b` ?
any good solution ?
3. Implementation:

it's not a big issue, but it gets tricky when implementing such that thing, any help here ?


so if anyone can make good solutions for these issues, and of course any PRs are more that welcomes

j-maas commented 6 years ago

Thanks for your explanation! I just wanted to check whether I didn't understand something, but I get why it's difficult to design and implement.

Just a quick remark: In your 2. example I would expect ModuleB to be accessible through both /a/b and /a/c. ;)

Thanks for your answer!

shekohex commented 6 years ago

So let's see if someone have a good idea to solve this issue, and it will be awesome if there is PR too 😀

jrista commented 5 years ago

In addition to nested routes keeping their prefix...I think it would also be very useful to allow routes that have nested children to be parameterized. I am not sure how this would need to work, and if I find the time I'd like to take a look at nest-router and see if I could fix/enhance it...but I need the ability to handle both parameter-less routes, as well as prameterized routes.

If I am looking up say users, and users can have comments. I might need the following routes:

/users?page&size: GET; list all users (with optional paging) /users: POST; create a user /users/:userId: GET;PUT;PATCH;DELETE; manage a single user /users/comments?page&size&startDate&endDate: GET; list all comments within date range (optional paging) /users/:userId/comments: GET; list all comments for user /users/:userId/comments: POST; create a comment for this user /users/:userId/comments/:commentId: GET;PUT;PATCH;DELETE; manage comments for this user

So there are potentially multiple parent/child relationships here. In one case, we have all comments for all users. In the other, we have all comments for a specific user. These relationships need to be defined rather explicitly, as trying to get all comments for all users, sorted by date and possibly broken into pages, by querying each user's comments one at a time is largely untennable.

I wonder if the following might be possible:

RouterModule.forRoutes([
      { path: '/users', module: UserModule },
      { path: '/users/comments', module: CommentModule },
      { path: '/users/:userId/comments', module: UserCommentModule },
    ])

Or, something along these lines. The modules would then be able to support all the necessary routes, each:

/users(/*) would the work with UserModule/UserController. If you need to list all the user, you would handle just /users, if you need to create a user you would handle just '/users', but if you need to get, update or delete a single user, you would handle /user/:userId'. Further, you would be able to do the same for the/users/comments(/*)path. The CommentModule/CommentController would be able to handle both/users/commentsas well as/users/comments/:commentIdif necessary. And of course the comments nested under/users/:userId/commentscould also handle/users/:userId/comments/:commentId`.

This kind of hierarchical API is very common for the projects I work on. We prefer to hierarchically relate entities as appropriate, and the same entities may even be accessible from more than one path. As an even deeper example, we my also have posts, which are created by users, and each of which have comments. We might need:

GET /posts POST /posts PUT;DELETE /posts GET /users/:userId/posts GET /posts/:postId/comments etc.

akash-gupt commented 5 years ago

@Y0hy0h @shekohex @jrista @tiggerk @greenkeeper[bot]

jordancue commented 5 years ago

@techaks were you able to get nested child route parameterization working? This is a requirement for us in our app as well.

TheShadowfax commented 5 years ago

@jordancue I've tried the example that you mentioned. It works fine but the swagger UI plugin doesn't properly prepare the URL as we expect for example when we try to do something as /posts/1001/comments it wont replace the :postId with the proper value it sends something as /opsts/:postId/comments but works fine when we prepare URL by ourselves in postman and btw the configuration was RouterModule.forRoutes([ { path: '/users', module: UserModule, children:[ { path: '/comments', module: CommentModule } ] } ])

jrista commented 5 years ago

Is there any official word on the full range of hierarchical routs with nestjs? I listed a number of possible use cases we often utilize ourselves in my previous post... Would be nice to know if/when all of those options (and, of course, deeper nesting to the nth level) would be supported by nestjs.

shekohex commented 5 years ago

Hi @TheShadowfax

It works fine but the swagger UI plugin doesn't properly prepare the URL as we expect

That's a bug in Swagger Module it self, see #3 i reopened that issue and made a PR fix to the upstream @nestjs/swagger package, hopefully kamil will merge it soon.

shekohex commented 5 years ago

Hi @jrista I'm a little bit confused, could you please clarify what you mean ?

And if I not mistaken, the nest-router package has no limit in how many deep your tree, of course the limit would be the stack overflow error, since the flatten function gets called on every level of your tree and flat all that to a single flat structure.

jrista commented 5 years ago

Well, I may have to try this again, but in the recent past with a project that has wrapped up, the following kind of use case did not seem to be well supported:

/users/comments?page&size&startDate&endDate: GET; list all comments within date range (optional paging)

If you normally have /users/:userId, and had comments nested under them, you would usually need to have a userId to get the comments. Trying to get all comments made by all users...that did not seem to be supported. We would usually end up creating something like /usercomments instead.

shekohex commented 5 years ago

@jrista, did you tried to do so ?

const routes: Routes = [
  {
    path: '/users',
    module: UsersModule,
    children: [
      {
        path: 'posts',
        module: PostsModule,
        children: [{ path: ':postId/comments', module: CommentsModule }],
      },
      { path: ':userId/comments', module: CommentsModule },
    ],
  },
]

and can you please take a look at this example

jrista commented 5 years ago

This may be the ticket:

      { path: 'nested/cats', module: CatsModule },
      { path: ':ninjaId/cats', module: CatsModule },

To map the same module twice...once with the parent id and once without.

Groxot commented 5 years ago

@jrista, did you tried to do so ?

const routes: Routes = [
  {
    path: '/users',
    module: UsersModule,
    children: [
      {
        path: 'posts',
        module: PostsModule,
        children: [{ path: ':postId/comments', module: CommentsModule }],
      },
      { path: ':userId/comments', module: CommentsModule },
    ],
  },
]

and can you please take a look at this example

https://stackoverflow.com/questions/57346320/how-split-routes-by-controllers-in-nestjs-router Can we move children array to another file ?