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

Issue: Incorrect route returned for dynamic segment when sibling catch-all segment exists #248

Closed zto-sbenning closed 1 month ago

zto-sbenning commented 1 month ago

Issue: Incorrect route returned for dynamic segment when sibling catch-all segment exists

Description: The method router.getRouteFromHref(dynamicSegmentHref) incorrectly returns the route for the catch-all segment instead of the route for the dynamic segment. This issue seems to be related to the sorting function used by the router. It's unclear whether this behavior is a bug or intended.

Steps to Reproduce:

  1. Define a route configuration where both a dynamic segment and a catch-all segment exist as siblings.
  2. Use router.getRouteFromHref(dynamicSegmentHref) with a URL that matches the dynamic segment.
  3. Observe that the route returned is for the catch-all segment rather than the dynamic segment.

Expected Behavior: router.getRouteFromHref(dynamicSegmentHref) should return the route associated with the dynamic segment when a URL matches it, even when a sibling catch-all segment exists.

Actual Behavior: router.getRouteFromHref(dynamicSegmentHref) returns the route for the catch-all segment instead of the route for the dynamic segment.

Possible Cause: The issue may be related to the sorting function used by the router, which might not be correctly prioritizing the dynamic segment over the catch-all segment.

Example Configuration: roots.config.js:

const path = require('path')

module.exports = {
    originDir: path.resolve(__dirname, 'src/roots'),
    localizedDir: path.resolve(__dirname, 'src/app/(routes)'),
    locales: ['en', 'cs'],
    defaultLocale: 'en',
    prefixDefaultLocale: false, // serves "en" locale on / instead of /en
}

src/roots/[dynamic]/page.tsx:

export default function DynamicPage() { return <></> }

src/roots/[...catchAll]/page.tsx:

export default function CatchAllPage() { return <></> }

src/roots/page.tsx:

import { PageProps, Router, schema } from "next-roots";

export default function Home({pageHref}: PageProps) {
    const router = new Router(schema);
    const locale = router.getLocaleFromHref(pageHref);
    const href = router.getHref("/[dynamic]", { locale, dynamic: "page" });
    const routeName = router.getRouteFromHref(href)?.name;
    return (
        <p>Dynamic routeName: "{routeName}"</p>
    );
}

Example Usage: Accessing home page shows: Dynamic routeName: "/[...catchAll]"

Expected Result: getRouteFromHref should returns "/[dynamic]".

Actual Result: getRouteFromHref returns "/[...catchAll]".

Suggested Fix: Review and adjust the sorting function used by the router to ensure that dynamic segments are correctly prioritized over catch-all segments when determining the route. If this behavior is intentional, please could you clarify the design decision ?

I can work on a PR for this.

Thank you!

svobik7 commented 1 month ago

There is already some sorting mechanism (see https://github.com/svobik7/next-roots/blob/5b659c0ab1da2ebc364a9fa48fab22cf359e36f3/src/router/router.ts#L76) but needs to be adjusted. We might need to push any [… to the end.

IMO we need:

  1. Non-dynamic routes/segments to come first
  2. Dynamic but not catch all as second
  3. Catch all as third

Probably we need to sort routes segment by segment with above rules? But that could harm performance

svobik7 commented 1 month ago

One thing cane to my mind - we can represent each segment as a number:

  1. static segment = 1
  2. dynamic = 2
  3. catch-all or optimal catch all = 3

For each route we can then compute its weight represented as decimal number like:

/static/[dynamic] = 0.12 /[dynamic]/[dynamic] = 0.22 /[dynamic]/static = 0.21 /[…catchAll] = 0.3

Then simple sort of decimal numbers can do the trick. We can even calculate this map only once.

What do you think?

zto-sbenning commented 1 month ago

Crystal clear! Geat idea.

I can propose a PR for this. I might need some help to rework the tests for the best. I'll tell you .

github-actions[bot] commented 1 month ago

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

The release is available on:

Your semantic-release bot :package::rocket: