sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
18.45k stars 1.89k forks source link

Populate matcher function with inherited slugs #4500

Open ghost opened 2 years ago

ghost commented 2 years ago

Describe the problem

I'll just describe my situation: My SvelteKit site is supposed to be localised into multiple (predefined) languages. To archive that I created this SvelteKit route layout: /src/pages/[...locale=locale]/[about=about].svelte. Implementing the locale slug was pretty easy thanks to the recent addition of matchers (In this case my matcher only returns true if [ "", "de", "lv" ] includes locale), but now comes the tricky part: I obviously also want to localise the urls of individual pages, but this isn't possible without knowing the locale slug.

Describe the proposed solution

match() could receive a second argument, called params. params would be an object with the same layout as event.params in handle().

export function match(param, params) {
    return {
        "": "about",
        "de": "uber",
        "lv": "par"
    }[params.locale] === param
}

Alternatives considered

Currently it should be possible to create a store to make inheriting possible. While this works it definitely is a workaround.

Importance

nice to have

Additional Information

No response

Mlocik97 commented 2 years ago

having function with params that are named nearly same aka param, params feels little overparametred... but I agree about feature, except we should rename one of these params. Maybe slug_param and event_params

ghost commented 2 years ago

Definitely. I just used params since event.params already exists, but obviously everything goes. Btw if anyone needs this: Adding a store does indeed work, just .set() it to be param before returning.

apttx commented 2 years ago

i'm trying the exact same thing (localising routes) but using handle in hooks.js. the issue i'm facing is different, though. i can successfully resolve the routeId and params from a localised route (with a custom, recursive tree), but modifying event does nothing because route parsing is done way before and out of my control (as far as i can tell).

i'm wondering if route could be passed to handle, modified by the user, and used in resolve_opts (defaulting to route). this would allow completely custom route resolution via optional overrides (which i know is not usually a concern, but crucial with i18n). rendering a default locale at unprefixed (/en vs /) would be trivial as well. all params are also known and available.

apttx commented 2 years ago

i'm wondering if route could be passed to handle, modified by the user, and used in resolve_opts (defaulting to route).

i've tried this and it works well. but i needed access to the manifest routes so i had to pass those in, too. it's a bit clunky and manual, but does the job. maybe there should be some sort of api that lets you select a route by route id?

 export const handle = ({ event, resolve, reroute }) => { 
   // event.url.pathname = /lv/par
  const resolvedRouteId = '[locale]/about' // up to the user, but maybe there could be some helpers

  const route = reroute(resolvedRouteId) // could automatically set the route in bg, but very side-effect-y. alternatively could get the desired route & pass it to resolve

  return resolve(event, null, route)
 }

i don't know how much interest there is in this now, but looking at #553, localised urls are a feature many need.

loup-brun commented 2 years ago

I think the issue described by @martinszar is spot-on for eventual i18n efforts (#553) in SvelteKit. Being able to validate a route semantically through relations (the example in the docs shows an example of checking digits, which is a very, very simple case of formal checking) would be helpful. Being able to do so in a load function (without having to end a request) feels more appropriate, because this kind of validation may include calls to local endpoints or external APIs (e.g. Does this section exist?, Is this lang supported by the CMS? and so on), all while depending on other route params which, perhaps ironically (or just by separation of concerns), ParamMatcher does not have access to.

Given a simple routes structure like this one:

routes/
 |- [lang]/
   # we can throw an early 404 here if `lang` isn’t supported (thanks to `load()`)
   |- +layout.server.js 

   # `sections` route depends on the `lang` parameter (which `ParamMatcher` does not have access to!)
   |- [...sections=availableSection]/
      |- +page.server.js
      |- +page.svelte
      |- [subPageSlug=availableSubPageSlug]/
         |- +page.server.js
         |- +page.svelte

   # `/my-section` route would default here -- which is not what we want!
   |- [pageSlug=availablePageSlug]/
      |- +page.server.js
      |- +page.svelte

… it is not obvious which of [sections] or [pageSlug] SvelteKit should resolve to—and yet, it will choose the lower-level [slug] early (because it has less rest params, as per the docs). So far, there isn’t a trivial way to specify route priority unless using a ParamMatcher—which has access only to the param it’s checked against and not other (potentially) crucial params.

I see two (very high level) solutions:

  1. ParamMatcher functionality could be incorporated into the load() function (in +layout.server.js), allowing to indicate that the current route does not apply to the current request rather than ending the request in a 404.
  2. Give some kind of additional load functionality to ParamMatcher, such as access to params as suggested by @martinszar and @Mlocik97.

This said, I’m not very found of the second solution.

I think the first solution would make more sense (again, from a very high-level, conceptual, perspective from a consumer of this software), allowing for API consolidation of the load function and therefore deprecating the params folder. I believe this kind of API change in SvelteKit would not only benefit eventual i18n support (which, for the time being, falls in the post-1.0 priority), but also just complex route matching scenarios (which might be 1.0-relevant).

At a glance, #5072 seems to point to the same problem, though it is filed under the 1.0 milestone.

rinart73 commented 1 year ago

This said, I’m not very found of the second solution.

From what I can see the second solution is much easier to implement by making the following changes: https://github.com/sveltejs/kit/blob/f7c0246a1dee8050b8a5defee86eb7c90242c692/packages/kit/src/utils/routing.js#L163

 if (!param.matcher || matchers[param.matcher](value, result)) { 

and https://github.com/sveltejs/kit/blob/f7c0246a1dee8050b8a5defee86eb7c90242c692/packages/kit/src/exports/public.d.ts#L939

 export type ParamMatcher = (param: string, matchedParams: Record<string, string>) => boolean;

So maybe for the time being it would be better to implement a fast solution and leave a big revamp that would introduce breaking changes for later?