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

Fixes matching of similar dynamic parameters #44

Merged

Conversation

ycs77
Copy link
Contributor

@ycs77 ycs77 commented Dec 19, 2024

Describe Bug

If I have below dynamic routes:

  • /blog/[slug]
  • /user/[slug]

When I change the language selector from above both routes, I'll always return the /blog/[slug].

Description

Original method to find the dynemic route is not working for finding two (or more) dynamic routes, because it just compare the route params keys (like slug) and return the first one:

.find(
(route) =>
JSON.stringify(route.params.sort()) ===
JSON.stringify(
Object.keys(config.paths.dynamicParams?.[locale] ?? {}).sort(),
),
);

If I have below dynamic routes:

  • /blog/[slug]
  • /user/[slug]

It will always return the /blog/[slug] because it's the first one.

So I've changed to the RegExp to match the dynamic routes.

Testing

I've add the /user/[slug] route into the playground.

You can follow the steps below to test:

  1. Clone the repo with this PR
  2. Compile the package and open the playground
  3. Visit http://localhost:4321/user/a
  4. Change the language selector to fr and you'll see the /user/[slug] route is returned
  5. If you revert the patch code back to orignal and repeat the steps, you'll see the /blog/[slug] route is returned

Copy link

netlify bot commented Dec 19, 2024

Deploy Preview for astro-i18n ready!

Name Link
🔨 Latest commit aba8415
🔍 Latest deploy log https://app.netlify.com/sites/astro-i18n/deploys/676a7fa9670d520008b2f712
😎 Deploy Preview https://deploy-preview-44--astro-i18n.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 19, 2024

Deploy Preview for astro-i18n-demo ready!

Name Link
🔨 Latest commit aba8415
🔍 Latest deploy log https://app.netlify.com/sites/astro-i18n-demo/deploys/676a7fa975a70500089d8c07
😎 Deploy Preview https://deploy-preview-44--astro-i18n-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@florian-lefebvre florian-lefebvre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@florian-lefebvre florian-lefebvre merged commit a2b75e9 into astrolicious:main Dec 24, 2024
9 checks passed
@ycs77 ycs77 deleted the fix-incorrect-matching-route branch December 24, 2024 10:23
@ycs77 ycs77 changed the title Fix incorrect matching second dynamic route Fixes matching of similar dynamic parameters Dec 24, 2024
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 this pull request may close these issues.

2 participants