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

feat: add new regex routing #2937

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

feat: add new regex routing #2937

wants to merge 18 commits into from

Conversation

s00d
Copy link
Contributor

@s00d s00d commented Apr 27, 2024

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

The new version of this commit #2927 includes a new routing system. However, I was unable to complete the last two tests:
specs/routing/prefix-and-default.spec.ts > localePath > route strategy: prefix_and_default > should be worked.
These are related to link generation. Unfortunately, I still can't figure out how it works or how to fix it.

Everything is functioning now, please take a look when you have time.

The description was in the previous pull request.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have added tests (if possible).
  • I have updated the documentation accordingly.

@s00d
Copy link
Contributor Author

s00d commented Apr 28, 2024

I've caught a memory leak when using routeToObject instead of:

Copy code
const subRoute = {
    name: route.name,
// ...

In this case, all tests start to lag. It seems like this issue might be relevant to any code where routeToObject is used

@s00d
Copy link
Contributor Author

s00d commented Apr 28, 2024

memory tests(10k requests)

regexp version
image

8.3.1
image

The bundle size on a real project with 8 locales, 20 pages, and 100 components.
regexp version
387kb
image

8.3.1
436kb
image

Now there can be even 100 locales and the bundle will not keep increasing indefinitely.

@s00d
Copy link
Contributor Author

s00d commented Apr 28, 2024

Revised Routing Description:

1. Primary Route:

  • Name: "blog"
  • Path: "/blog"
  • Description: This is the base route, unchanged from the original setup. Modifications can be applied based on the chosen strategy, usually serving as an integral component of the routing structure.

2. Localized Route:

  • Path: "/:locale(fr)/blog"
  • Name: "blog___locale"
  • Description: This route type integrates regular expressions for locales directly into the URL path, and the route name is prefixed with ___locale. This setup facilitates serving content in multiple languages, improving accessibility and user experience in different regions.

3. Custom Routes:

  • Path: "/:locale(fr)/produits/:id()"
  • Name: "products-id___locale___fr"
  • Description: Custom routes are prefixed with locale identifiers and typically require more complex logic and extensive testing. Their implementation and behavior can vary based on the routing strategy employed.

Additional Enhancements:

  • Meta Tag: The meta.locale attribute is added to all routes to check if the module interacts properly with the route.
  • Route Search: The system implements a search mechanism for routes based on the route name, locale, and strategy. This process is designed to identify the most efficient routing pattern, although it may not always be optimal.

Code Explanation:

  • Localized Route Configuration:

    • The localizeRoutes function dynamically adjusts routes based on localization settings, employing strategies such as prefixing routes or excluding the default locale prefix based on the configuration.
    • Routes are adapted to include or exclude locale prefixes, and special cases are handled where routes need to accommodate configurations like trailing slashes or domain-specific defaults.
  • Route Localization Logic:

    • The function localizeRoute iterates over routes, applying localization settings based on the route's properties and specified options. This includes handling exceptions for redirect routes and integrating locale-specific paths and names.
    • The configuration also allows for nested routes and children of localized routes to inherit or override the parent localization settings.

Route Search Process:

  • The resolveRoute function is crucial for determining the appropriate localized route based on the current locale, route specifications, and routing strategy. It evaluates whether the route parameter is a string representing a path or a route name and adjusts accordingly.
  • If the route is identified as a named route, it resolves to a localized name considering the default locale and current strategy, incorporating the locale as a parameter if required by the strategy.
  • For path routes, the function adjusts the path to include the locale prefix if the strategy dictates, and ensures the path conforms to configurations such as trailing slashes.
  • The search logic also includes error handling to return null if no matching route is found, maintaining robustness in the routing process.

@s00d
Copy link
Contributor Author

s00d commented Apr 29, 2024

I've fixed all the tests, but there are still some warnings left that I couldn't catch locally; my tests don't show them. Maybe you'll be able to catch them. The resolve function sometimes passes the locale parameter where it's not needed (to routes without a locale), but it's already possible to start reviewing and testing.

I also had to adjust two tests. One issue was related to i18n, and for the second one, I'm out of energy to fix it.

@junemolison
Copy link

junemolison commented Jul 15, 2024

I was wondering if there might be an update on the timeline for merging this? Thank you for your time and consideration.

@BobbieGoede
Copy link
Collaborator

I was wondering if there might be an update on the timeline for merging this? Thank you for your time and consideration.

I was a bit busy the last few months but I'm still considering this feature as we're working v9, no timeline though.

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.

3 participants