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

Support suffixes for [...rest] endpoints #4656

Closed
ngalaiko opened this issue Apr 19, 2022 · 11 comments
Closed

Support suffixes for [...rest] endpoints #4656

ngalaiko opened this issue Apr 19, 2022 · 11 comments
Labels
bug Something isn't working p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc.
Milestone

Comments

@ngalaiko
Copy link

ngalaiko commented Apr 19, 2022

Describe the problem

I have a project with a following pages routes structure:

/routes/index.svelte
/routes/posts/index.svelte
/routes/posts/index.json.js
/routes/posts/category1/index.svelte
/routes/posts/category2/index.svelte
/routes/posts/[...catchall]/index.json.js
/routes/posts/[...catchall]/index.svelte

What I am doing there:

  • /routes/posts/index.{svelte,json.js} is a list page with every post
  • /routes/posts/category*/**/*.svelte are posts with just markup and some props
  • /routes/posts/[...catchall]/index.svelte fires when no post if found, so it queries /routes/posts/[...catchall]/index.json.js to check if the post has an alias and redirects

What I would like to do is to add a generic /likes.json endpoint to all the posts, so:

...
/routes/posts/[...catchall]/likes.json.js
/routes/posts/[...catchall]/index.svelte
/routes/posts/[...catchall]/index.svelte

But the problem I am facing is that the request is routed to the /routes/posts/[...catchall]/index.svelte handler, even though according to sorting rules, likes.json must have a priority

Describe the proposed solution

Support catchall suffixes

Alternatives considered

I could solve this without having [...rest] matcher of course, but that involves either copy-paste, or manual routing.

Importance

would make my life easier

Additional Information

This seems like an improvement of an existing feature

@Rich-Harris Rich-Harris added the bug Something isn't working label Apr 20, 2022
@Rich-Harris Rich-Harris added this to the 1.0 milestone Apr 20, 2022
@Rich-Harris
Copy link
Member

There is a bug here — because the client-side routing table doesn't include endpoints, the router doesn't 'see' that /posts/[...catchall]/likes.json.js is a better match for /posts/a/b/c/likes.json than /posts/[...catchall]/index.svelte. We might have to include endpoints in the routing table to fix that.

That said, how are you running into this bug? If you hit /posts/a/b/c/likes.json directly, or fetch it, the data will come directly from the server. If you're linking to the JSON file for some reason, you can workaround the bug by putting sveltekit:reload on the link. Is there some other way you're being sent to the wrong route?

@Conduitry
Copy link
Member

We had previously (#2656) decided to not tell the client-side router about endpoints, and had said if you wanted to link to them, you'd need sveltekit:reload. If we're sticking with that, this doesn't sound like a bug I don't think. Are you considering revisiting that now that we've overhauled routing a bit and eliminated route fallthrough?

@Rich-Harris
Copy link
Member

No, I just forgot we'd made that call. Now that we've eliminated fallthrough, it's possible that we could reinstate the previous behaviour in a less costly way — maybe it's possible to only tell the client-side router about ambiguous cases like this one, rather than all endpoints.

@Conduitry
Copy link
Member

Having the client-side router know about endpoints (and use a full-page navigation in that case) only when there's an ambiguity - but to have it continue to show the client-rendered 404 page when linking to any other endpoint - sounds confusing to me. That's worse than either consistently having it know about endpoints or consistently having it not know about endpoints.

@mrkishi
Copy link
Member

mrkishi commented Apr 20, 2022

Unmatched routes already trigger a full page navigation by default; the client only renders 404s for matched page routes, hence the bug with ambiguous routes.

@Conduitry
Copy link
Member

Oh! I had forgotten about that change. Yup, makes sense to just include this for potentially ambiguous endpoints.

@ngalaiko
Copy link
Author

ngalaiko commented Apr 21, 2022

Thanks for looking into it.

Here is a reproduction sandbox: https://stackblitz.com/edit/sveltejs-kit-template-default-pkjzkn?file=src/routes/index.svelte

If you hit /posts/a/b/c/likes.json directly, or fetch it, the data will come directly from the server. If you're linking to the JSON file for some reason, you can workaround the bug by putting sveltekit:reload on the link. Is there some other way you're being sent to the wrong route?

That actually doesn't seem to be the case, that's why I opened it.

I want to be able to both query the endpoint myself with let's say curl, and also use it inside a load() function to pretender data in svelte kit

@Rich-Harris
Copy link
Member

Ah okay, that seems to be a completely different bug — index.json.js is being sorted before likes.json.js in the server-side routing table.

@elikoga
Copy link
Contributor

elikoga commented Jul 13, 2022

Found the issue:

// xy < x[y], but [x].json < [x]
if (pa === undefined) return pb.dynamic ? -1 : +1;
if (pb === undefined) return pa.dynamic ? +1 : -1;

This comparison function does the sorting of values.

a.segments: [[{"...stuff",dynamic},{".json",not-dynamic]]
a.id: [...stuff].json

b.segments: [[{"...stuff",dynamic}][{"specific.json",not-dynamic]]
b.id [...stuff]/specific.json

Since The first argument has more parts in the first segment, it wins.

Can be fixed by adjusting compare(a,b) or id, segments logic

@benmccann benmccann added the p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc. label Jul 19, 2022
@Rich-Harris Rich-Harris modified the milestones: 1.0, whenever Jul 20, 2022
@benmccann benmccann added p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc. and removed p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc. labels Jul 20, 2022
@elikoga
Copy link
Contributor

elikoga commented Aug 2, 2022

This is impacted by #5748 and could possibly be closed by that?

@Rich-Harris
Copy link
Member

indeed it can :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc.
Projects
None yet
Development

No branches or pull requests

6 participants