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

load fall-through #548

Closed
Rich-Harris opened this issue Mar 16, 2021 · 18 comments
Closed

load fall-through #548

Rich-Harris opened this issue Mar 16, 2021 · 18 comments

Comments

@Rich-Harris
Copy link
Member

Suppose you had the following routes:

src/routes/[animal]/index.svelte
src/routes/[vegetable]/index.svelte
src/routes/[mineral]/index.svelte

SvelteKit would throw an error on encountering these because they conflict — there's no way to know, given a URL like /potato, whether it's an animal, a vegetable, or a mineral.

Our half-hearted solution to this is to allow a subset of regex syntax in filenames. But no regex exists that would allow you to disambiguate between those routes, and even when regex is suitable, the limitations of filenames (e.g. no \d or \w) render them somewhat useless.

Here's an alternative idea:

<!-- src/routes/[animal]/index.svelte -->
<script context="module">
  export async function load({ page, fetch }) {
    const res = await fetch(`/api/animals/${page.params.animal}`);

    if (res.status === 404) {
      // not an animal — return nothing, to indicate that we should fall through to vegetables
      return;
    }

    if (res.ok) {
      return {
        props: { animal: await res.json() }
      };
    }

    return {
      status: res.status,
      error: new Error((await res.json()).message)
    };
  }
</script>

If all matching routes had load functions that returned falsy values, SvelteKit would render the error page with a generic 404.

This isn't perfect: it requires the browser to load src/routes/[animal]/index.svelte and all its dependencies, only for SvelteKit to discover it's not what we wanted, potentially slowing navigations down unacceptably. But it gives us a degree of power and flexibility that isn't possible any other way.

We can mitigate that by loading all matching routes in parallel, for the rare cases where there's more than one. The downside is that you might end up fetching more code than is necessary, but I think it's probably an acceptable trade-off.

If we had this, would we ditch regex routes? They allow routes to be rejected earlier, but as mentioned have limitations that make them pretty useless anyway.

@GrygrFlzr
Copy link
Member

This seems like in the all-failure case, we'd end up with a cryptic 404, because none of the errors we throw from the routes show up? Or do we merge the stacktrace somehow? If we load in parallel, what happens when two or more pages respond with a non-error? What mechanism do we use to identify the order in which the fall through happens?

@Rich-Harris
Copy link
Member Author

Rich-Harris commented Mar 16, 2021

This seems like in the all-failure case, we'd end up with a cryptic 404, because none of the errors we throw from the routes show up?

If a load function returns anything, including { error: whatever }, then we don't fall through

What mechanism do we use to identify the order in which the fall through happens?

Alphabetical? It isn't the most 'semantically correct' solution but if you really needed to check vegetables before minerals before animals you could do

src/routes/[a_vegetable]/index.svelte
src/routes/[b_mineral]/index.svelte
src/routes/[c_animal]/index.svelte

The advantage of alphabetical instead of something out-of-band is that you can see the expected behaviour just by looking at your editor's file tree

@benmccann
Copy link
Member

If a page does not define a load function then that's another case where I would also think that we should probably not fall through

@GrygrFlzr
Copy link
Member

Alphabetical?

This has some gotchas.

Windows Explorer sort order:

[1-mineral].svelte
[2-vegetable].svelte
[11-animal].svelte

image

Unix sort order:

[11-animal].svelte
[1-mineral].svelte
[2-vegetable].svelte

image

VS Code sort order on my Windows machine
image

@Rich-Harris
Copy link
Member Author

This has some gotchas.

this is why we can't have nice things 😭 . Though given how rare it would be to use numbers in this context, I think we can probably just disallow it

If a page does not define a load function then that's another case where I would also think that we should probably not fall through

agree

@Conduitry
Copy link
Member

I'd suggest making the separator between the sort-thing and the slug name be a character that can't appear in variable names. This way we can easily strip it out of the param name. It seems more palatable to say that .../[a-foo]/... means params.foo than it does to say that .../[a_foo]/... always means params.a_foo or that it always means params.foo.

@Conduitry
Copy link
Member

A not-insignificant benefit of completely dropping regex routes is that it would make #484 go away instead of being the small nightmare it is almost certain to be.

@Rich-Harris
Copy link
Member Author

It seems more palatable to say that .../[a-foo]/... means params.foo than it does to say that .../[a_foo]/... always means params.a_foo or that it always means params.foo.

huh, I'd imagined that a-foo would be params['a-foo']. are you suggesting enforcing /^[a-z]+$/ parameter names?

@Conduitry
Copy link
Member

All of /^[^-]+$/ maybe? I'm not sure.

If you'd rather have the things between the brackets be the literal param names, that's fine. We can always rename them when destructuring args to the load function if we don't like the names, or if we don't want the sort key part. It would definitely be fewer moving parts.

@benmccann
Copy link
Member

limitations of filenames (e.g. no \d or \w)

I'm not aware of any such limitation on Ubuntu OS. Is this from Mac or Windows or elsewhere in SvelteKit?

@Conduitry
Copy link
Member

Yes, file and directory names can't have \ / : * ? " < > | in them in Windows. I don't know about macOS.

@benmccann
Copy link
Member

Oh right. Also, I sort of evaluated the regex in my head when reading the comment and thought the comment was saying there couldn't be spaces or numbers 😄

With the goal of minimizing unnecessary data transfer, I wonder if there's another way we could structure this. E.g. we could have src/routes/[animal-or-veg-or-mineral].svelte which the user could code to dynamically choose the relevant template. I think the main difficulty here is that with code splitting it could introduce two round trips. One to get to [animal-or-veg-or-mineral].svelte and then one to get to the final destination. I wonder if we could perhaps add some way for the user to specify that [animal-or-veg-or-mineral].svelte should always be prefetched

@Rich-Harris
Copy link
Member Author

That mechanism already exists:

<script context="module">
  export async function load({ page, fetch }) {
    let Page;
    switch (get_type(page)) {
      case 'animal':
        Page = (await import('./_Animal.svelte')).default;
        break;
      case 'mineral':
        Page = (await import('./_Mineral.svelte')).default;
        break;
      case 'vegetable':
        Page = (await import('./_Vegetable.svelte')).default;
        break;
      default:
        return; // not found
    }
    return {
      props: { Page }
    }
  }
</script>

<svelte:component this={Page}/>

It's not exactly ergonomic but it works, no need to introduce some other mechanism. In the majority of cases loading both candidate routes simultaneously is fine, I think, so this would be a rare case

@Conduitry
Copy link
Member

Implemented in #583.

@Evertt
Copy link

Evertt commented Apr 4, 2021

If a load function returns anything, including { error: whatever }, then we don't fall through

@Rich-Harris why do it like this though? Why not just use { status: 404 } to trigger a fall-through? If there's only one page and it returns a 404, we render a 404 page with the specific error message from that page. If there are more pages and all of them return a 404, then we end up rendering a generic 404 page. Doesn't that sound good?

I'm asking because at the moment I'm working on a page that uses the load() function, but it doesn't need to return any props from it. And now it feels a bit silly to have to return { props: {} }.

@Conduitry
Copy link
Member

We want a route to be able to say 'I know this is intended for me and I know it's a 404' and not have to check any other routes. I'd expect you should be able to return just {} from load and have that indicate you want to handle it.

@Evertt
Copy link

Evertt commented Apr 4, 2021

Ah sorry, I hadn't realized that returning {} would also work. That's already a lot less clunky.

@shaozi
Copy link

shaozi commented Nov 16, 2021

I have a page named routes/request/edit-[id].svelte, but when the url is /request/edit- it is not matched. and further it triggered a fall through error. Is that as designed?

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

6 participants