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

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

Closed
Evertt opened this issue Jun 13, 2024 · 8 comments

Comments

@Evertt
Copy link

Evertt commented Jun 13, 2024

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
Copy link
Owner

jycouet commented Jun 13, 2024

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
Copy link
Author

Evertt commented Jun 13, 2024

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
Copy link
Owner

jycouet commented Jun 13, 2024

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
Copy link
Owner

jycouet commented Jun 13, 2024

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
Copy link
Author

Evertt commented Jun 15, 2024

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 made sure that the root route is already in a routing-group folder, namely `routes/(marketing)/+page.svelte
  • I also added +layout.svelte and +layout.ts files to routes/(marketing)/blog and routes/(marketing)/blog/(posts).

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

@jycouet
Copy link
Owner

jycouet commented Jun 16, 2024

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

I'll try to do the repro also in #668 (but not right now ;))

@jycouet
Copy link
Owner

jycouet commented Jul 29, 2024

You have any update here?

@jycouet
Copy link
Owner

jycouet commented Sep 11, 2024

Feel free to re open if you have news 😉

@jycouet jycouet closed this as completed Sep 11, 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

No branches or pull requests

2 participants