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

replace regex routes with fall-through #583

Merged
merged 18 commits into from
Mar 22, 2021
Merged

replace regex routes with fall-through #583

merged 18 commits into from
Mar 22, 2021

Conversation

Rich-Harris
Copy link
Member

So far, this only removes the regex routes, it doesn't replace them

@Rich-Harris
Copy link
Member Author

Turns out to be a slightly bigger PR than i expected. I think i'm close.

One consequence of this change is that you can't try a request as an endpoint request and then as a page request; they could be interleaved. So instead of separating pages from endpoints in the manifest, they need to be combined in a single list.

Other consequences:

  • when there was a 1:1 relationship between regexes and routes, it was straightforward to respond with a 501 for routes that existed but didn't implement the request method. That's a little trickier now (not impossible I guess, though the semantics aren't 100% clear to me) so at least for now those requests result in a 404 instead.
  • we can now have endpoints that 'shadow' pages. Since /foo.js has a higher precedence than /foo.svelte, you could (for example) implement POST /foo which writes to a database, but doesn't return anything, causing the request to fall through to the page. This reinstates behaviour that was present in Sapper but dropped for Kit, and potentially addresses other esoteric requirements
  • the navigating store gets a little tricker. before, it was possible to set it to { from, to } where from and to are both objects of the form { host, path, params, query } as soon as the navigation started, because we could convert path to params synchronously. Now that we have to wait to see if the first match falls through, we can't do that. I'm not totally sure what the correct solution is — do we leave to.params undefined until we know it? or leave to undefined? Or something else?

On the server, we can try routes sequentially until we get a match. On the client that's less ideal — you don't want to have to go back to the network if the first route fails to match. But you also don't want to hit the network eagerly for every possible route, since that could mean a lot of network requests — if you have routes/specific_thing.svelte alongside routes/[everything_else].svelte, you probably don't want to load the latter if the user clicks on a link to /specific_thing.

So my plan is to implement a heuristic: if the first route we try has a pattern that is shared by other routes, we preload them. If you have

src/routes/a.svelte
src/routes/[b]-[c].svelte
src/routes/[d].svelte
src/routes/[e].svelte

then hitting /a would cause nothing to be preloaded. Hitting /x-y would cause nothing to be preloaded, because the first match ([b]-[c].svelte) doesn't share a pattern with any other routes. If [b]-[c].svelte didn't respond to /x-y then we'd fall through to [d].svelte.

But if you hit /foo then it would preload [d]-svelte and [e].svelte, because it's equally likely to match either.

@Rich-Harris Rich-Harris marked this pull request as ready for review March 22, 2021 16:10
@Conduitry
Copy link
Member

That's an upsetting test failure after successful tests. Is there something nondeterministic about the fallthrough behavior?

@benmccann benmccann linked an issue Mar 22, 2021 that may be closed by this pull request
@Rich-Harris
Copy link
Member Author

Fallthrough is deterministic. I think that test is non-deterministic though, it has a page.waitForTimeout in it. I vote we disregard it for now, and if it fails again, figure out how to make it deterministic

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

Successfully merging this pull request may close these issues.

Robustify regex routes
2 participants