svobik7 / next-roots

Next.js utility to generate i18n routes in the new APP directory.
https://next-roots-svobik7.vercel.app/
MIT License
178 stars 12 forks source link

Feature Request: Allowing "Generate params from the top down" for route with multiple dynamic segments #242

Closed zto-sbenning closed 1 month ago

zto-sbenning commented 1 month ago

Hi,

In the NextJS documentation, their is two ways to handle Multiple Dynamic Segments in a Route .

Generate params from the bottom up As I understand, this is what the generated code by next-roots expects.

Generate params from the top down As I understand, this is not possible with the current generated code, because it does not pass the received params.

I think it would be nice to add this.

From my point of view, it would require modifying the generated code for generateStaticParams from:

export async function generateStaticParams() {
  return generateStaticParamsOrigin({ pageLocale: "en" })
}

to:

export async function generateStaticParams(props: ATypeToBeDefined) {
  return generateStaticParamsOrigin({ ...props, pageLocale: "en" })
}

And also modifying the type for GenerateStaticParamsProps and for geting strong typing among params.

What is your thought on this ? I can try to do a PR for this if needed.

Cheers,

svobik7 commented 1 month ago

Hi @zto-sbenning,

I checked your proposal and it makes sense to me. Thank you for pointing this out 👍

It would be great if you can create a PR for this.

Please let me know in case of any guidance needed 🙂 but seems straightforward.

zto-sbenning commented 1 month ago

Hi,

I would be very happy to contribute to your project with this PR. I tried it, and it appears to be deeper than what I initially thought.

In fact, there is an open issue on Nextjs to fully support this "top down" pattern:

The workaround to be able to use this pattern is to use generateStaticParams from the layout file of the parent dynamic segment instead of using it in it's page file.

But it appear that next-roots is not generating this function for layout files (correct me if I'm wrong!).

So, I'm wondering if it best to open a new issue, and another PR for this, or should I propose a PR for this issue (issue-242) handling both:

Happy to have your thought on this.

svobik7 commented 1 month ago

I would prefer to split it to 2 issues. If I am not mistaken we can do the layout PR but need to wait with parent params PR. Just because "passing parent params" is dependant on that nextjs API change that is yet to be merged. IMO better to wait for them to fix it and update next-roots after that.

What do you think?

zto-sbenning commented 1 month ago

Tank you for the quick feedback!

Well, it is not really dependant. As for now, we can still access the parent params in the page getStaticParams if (and only if - due to the issue on nextjs) it was provided by the parent layout.

So if we can do both PR right now, we can acheive "Generate params from the top down" the same way plain Nextjs is currently doing (from layouts to pages).

But I totally agree for making it in two separate PRs.

Ok for you ?

svobik7 commented 1 month ago

I see, ok for me 👍

github-actions[bot] commented 1 month ago

:tada: This issue has been resolved in version 3.10.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: