jycouet / kitql

A set of standalone tools to SpeedRun WebApps!
https://kitql.dev
MIT License
401 stars 37 forks source link

`vite-plugin-kit-routes` generates incorrect routes when there could be multiple matches #665

Open Evertt opened 2 months ago

Evertt commented 2 months ago

Describe the bug

So, let's say I have these weird routes:

image

As you can see, it's ambiguous where /blog would route to. It could route to routes/(marketing)/blog/+page.svelte or it could route to routes/(marketing)/blog/(posts)/[[id=integer]]/+page.svelte.

According to the SvelteKit docs, when multiple matches are possible, SvelteKit will apply a sorting logic to decide which route to actually match with. In this case, /blog goes to routes/(marketing)/blog/+page.svelte.

Anyway, what the plugin generates, is this:

const PAGES = {
    "/blog": `/blog`,
    "/blog": (params?: {
        id?: Parameters<typeof import("../params/integer.ts").match>[0]
    }) => {
        return `/blog${params?.id ? `/${params?.id}` : ""}`
    },
}

Which of course throws this error:

An object literal cannot have multiple properties with the same name.

But of course, what it should have done is to pretend that id is not an optional route parameter. Because then it would've generated this:

const PAGES = {
    "/blog": `/blog`,
    "/blog/[id=integer]": (params: {
        id: Parameters<typeof import("../params/integer.ts").match>[0]
    }) => {
        return `/blog/${params.id}`
    },
}

Severity

annoyance

Steps to Reproduce the Bug

Create some route structure that creates ambiguity / multiple possible matches and see what the plugin generates.

Reproduction

No response

jycouet commented 2 months ago

Thanks @Evertt for reporting this issue!

I'm not sure we should move the optional route parameter to a non-optional one, as that might complicate things further.

Instead, what about ensuring that there are no multiple properties with the same name? We could generate unique keys like /blog, /blog_alternative_2, /blog_alternative_3, etc.

What do you think?

Evertt commented 2 months ago

Instead, what about ensuring that there are no multiple properties with the same name? We could generate unique keys like /blog, /blog_alternative_2, /blog_alternative_3, etc.

What do you think?

Oh please no. 😅 How about that in the case that you have both a route /blog and a route /blog/[[optional_parameter]], that you skip the generation of the /blog route, because it will already be covered by /blog/[[optional_parameter]].

So this:

const PAGES = {
    "/blog": `/blog`,
    "/blog": (params?: {
        id?: Parameters<typeof import("../params/integer.ts").match>[0]
    }) => {
        return `/blog${params?.id ? `/${params?.id}` : ""}`
    },
}

Turns into this:

const PAGES = {
    "/blog": (params?: {
        id?: Parameters<typeof import("../params/integer.ts").match>[0]
    }) => {
        return `/blog${params?.id ? `/${params?.id}` : ""}`
    },
}
jycouet commented 2 months ago

That's sounds great ! Thx for the proposal.

I'll look at it tonight 🥳 (unless you wanna do it, comment here next few hours)

jycouet commented 2 months ago

Actually, trying to reproduce, I get this error message from SvelteKit image

It seems that my repro is not correct... image

What I'm I missing?

Evertt commented 2 months ago

Hey, that's interesting.

So I just created a repro on stackblitz and what's interesting is that initially I got the same error from SvelteKit as you. But then when I made the routes folder resemble the one I had more accurately then suddenly SvelteKit's error disappeared.

https://stackblitz.com/edit/sveltejs-kit-routing-bug-repro

Which leads me to believe that maybe SvelteKit is actually supposed to throw the same error again, and that it's in fact a bug on their side that they don't throw the error in every case that they should?

edit

To make clear what changes I made to make SvelteKit's error go away:

I'm not sure which of those changes did the trick, but my guess is the first one.

jycouet commented 2 months ago

Looking at it quickly, it seems that the (group) is removing the warning.

I'll try to do the repro also in https://github.com/jycouet/kitql/pull/668 (but not right now ;))

jycouet commented 1 month ago

You have any update here?