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

Nest routing issue with nested pages #93

Closed
rubenvereecken opened this issue Nov 1, 2022 · 15 comments · Fixed by #96
Closed

Nest routing issue with nested pages #93

rubenvereecken opened this issue Nov 1, 2022 · 15 comments · Fixed by #96

Comments

@rubenvereecken
Copy link

Describe the bug
It's common to have page structures such as the following

- pages
  - index.tsx
  - users
    - index.tsx
    - [id].tsx

I have a simple UsersController that takes care of rendering all pages in pages/users. However, the following doesn't work:

@Controller('users')
export class UsersRenderController {
  constructor(private usersService: UsersService) { }

  @Get('/')
  @Render('users/index')
  async showAll() {
    return {};
  }

The result is a 404.

Strangely, when renaming the file pages/users/index.tsx to e.g. pages/users/users.tsx and changing the @Render line appropriate, that does work! However, it would mess up rendering for Next.js.

This seems quite an obvious one, so what am I missing here? None of the examples contain nested pages, so it's been hard to sanity-check.

Expected behavior
It should be possible to render nested pages as in the example above, e.g. pages/users/index.tsx.

To Reproduce
I can share the repo with someone privately, but it should be very simple to copy the above example in an existing repo.

Version

  • next.js: 12.3.1
  • nest: 9.0.0
  • nest-next: 10.0.0
@rubenvereecken
Copy link
Author

@yakovlev-alexey you seem to know your way around the codebase really well — perhaps you would see the problem more easily?

@rubenvereecken
Copy link
Author

@yakovlev-alexey
Copy link
Contributor

Pretty sure Next will try to parse index as the id.

Give this a try (do not specify index file explicitly, just the nested directory):

  @Get('/')
  @Render('users')
  async showAll() {
    return {};
  }

This was referenced Nov 2, 2022
@rubenvereecken
Copy link
Author

@yakovlev-alexey I see.. that explains a fundamental misunderstanding of mine: Nest is actually sending this to Next.js to interpret. It's not pointing at a particular file.

This also explains the second, bigger issue. How would I handle dynamic routes?

  @Get(':id')
  @Render('users/[id]')
  async showSingle(@Param() params) {
    console.log(`Route for single user`)
    return {};
  }

This doesn't work of course. How do I parameterize the page that's being rendered, so that I can actually pass on the id? Because in this case I can only get it to render properly if I use something literal like @Render('users/1').

@yakovlev-alexey
Copy link
Contributor

Pretty sure the issue was introduced in Next@12. I have an idea on how to fix this (though this might be a breaking change). I will get back with more info in an hour or two.

@kyle-mccarthy
Copy link
Owner

Hey it seems like your example is similar to the basic example. It has a nested routes for the blog, /blog which is the index and then /blog/[slug] which is the individual blog pages. Check out the controller here https://github.com/kyle-mccarthy/nest-next/blob/master/examples/basic/src/blog/blog.controller.ts

@rubenvereecken
Copy link
Author

@kyle-mccarthy I feel silly for not having found that example — that's exactly it.

Interestingly, that doesn't work. Next.js is interpreting this not as users/1 but as a very literal users/[id]. Like @yakovlev-alexey said, that might be a Next@12 thing.

@kyle-mccarthy
Copy link
Owner

@rubenvereecken can you provide a minimal reproducible example? I've verified that the basic example works with next 12. Perhaps it's something specific to your setup?

@yakovlev-alexey
Copy link
Contributor

@kyle-mccarthy check out #97. It includes a test app which serves as a reproduction repo. I've left a link to mirror PR in my fork where CI was able to run. And I can confirm that since Next@12 nested dynamic parameters don't work.

@rubenvereecken
Copy link
Author

@yakovlev-alexey any way I can help or contribute to the PR for the routing part?

@yakovlev-alexey
Copy link
Contributor

I think the routing issue should be fixed (apart from any route eg [...all]) with #96. Generally there are quite a few scenarios that need to be working. If you could take a look at #97 and provide feedback on the scenarios I provided (whether you see them as sufficient or maybe I missed some important ones). Otherwise I believe whats left to do is merge test scenarios, rebase #96 (and tidy up the code) and publish the new version. Currently only @kyle-mccarthy is capable of merging and publishing releases. Perhaps if Kyle can dedicate very limited time to this project it could be in everyones interest to have a CI Action to publish a new version and someone else with merge/tag publish button access.

@rubenvereecken
Copy link
Author

@basselabbara Can I get your take on #96?

@basselabbara
Copy link

@rubenvereecken @yakovlev-alexey I'm still checking the code and connecting the dots. I can see some console logs left. Is that on purpose?

@yakovlev-alexey
Copy link
Contributor

Logs definitely are not on purpose and are debugging leftovers - will remove them. Also thinking about renaming next directory to vendor/next. Probably will update the PR now

@yakovlev-alexey
Copy link
Contributor

PR is updated. Also thrown in Vercel Next.js license just in case.

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

Successfully merging a pull request may close this issue.

4 participants