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

Nested routes do not keep prefix #21

Open
j-maas opened this issue Jun 22, 2018 · 18 comments
Open

Nested routes do not keep prefix #21

j-maas opened this issue Jun 22, 2018 · 18 comments

Comments

@j-maas
Copy link
Contributor

j-maas commented Jun 22, 2018

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
Copy link
Contributor Author

j-maas commented Jun 22, 2018

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
Copy link
Member

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 nestjs/nest#255 (comment)

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
Copy link
Contributor Author

j-maas commented Jun 22, 2018

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

j-maas commented Jun 25, 2018

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
Copy link
Member

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
Copy link

jrista commented Feb 18, 2019

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
Copy link

@jordancue
Copy link

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

@TheShadowfax
Copy link

TheShadowfax commented Jun 12, 2019

@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
Copy link

jrista commented Jun 12, 2019

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
Copy link
Member

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
Copy link
Member

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
Copy link

jrista commented Jun 12, 2019

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
Copy link
Member

shekohex commented Jun 13, 2019

@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
Copy link

jrista commented Jun 17, 2019

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
Copy link

Groxot commented Aug 4, 2019

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

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

No branches or pull requests

7 participants