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

Additional option(s) for trailingSlash #1405

Closed
Rich-Harris opened this issue May 10, 2021 · 7 comments
Closed

Additional option(s) for trailingSlash #1405

Rich-Harris opened this issue May 10, 2021 · 7 comments
Milestone

Comments

@Rich-Harris
Copy link
Member

Is your feature request related to a problem? Please describe.
The trailingSlash options introduced in #1404 don't make it straightforward to add trailing slashes to index pages but not to leaf pages. For example you might want to have /blog/ and /blog/some-post rather than /blog and /blog-some-post, since that allows the index page to contain relative links.

Describe the solution you'd like
One possibility is to add a "smart" option with the semantics described in either #733 (comment) or #733 (comment):

  • any index.svelte file gets a trailing slash if there are also non-index pages in the directory, or in child directories
  • the more deterministic alternative: all index.svelte files get a trailing slash (which dictates code organisation decisions, but is probably the decision we'd regret less)

There is one problem, described in #733 (comment):

There's another wrinkle, which is that the index-ness of a URL can't actually be determined immediately, in the general case. Consider a scenario in which you have a /foo/[a]/index.svelte and a /foo/[b].svelte. We can't know whether /foo/x should refer to the first (in which case it should be redirected to /foo/x/?) or the second (in which case any trailing slash should be removed) without actually loading them and seeing if the first falls through to the second. (We could redirect at this later stage, but we'd then have to re-run load functions after the redirect had occurred, which is obviously undesirable. For the same reason, I don't think we can realistically have a way of configuring this at a page level; it needs to be an app-wide setting.)

For that reason the smart option would have failure cases — rare, and presumably identifiable-at-build-time failure cases, but failure cases nonetheless.

So another option is "strict", which would have the semantics describe above but wouldn't cause redirects: a request for /blog would 404 if the correct path was /blog/. We could implement one, both or neither.

Describe alternatives you've considered
The "ignore" setting provides an escape hatch for userland (or upstream, e.g. at the reverse proxy level) solutions, which gives us the option of not worrying about solving this problem. We could also just declare it to not be a problem, and settle for "never" and "always" being the only options that will cause an intervention by SvelteKit (as Next does, for example).

Per-page settings are also a possibility, though this has major drawbacks as described above.

How important is this feature to you?
Personally I'm good with "never" and "always", but I can definitely see the argument for /blog/. I'd rate it as a nice-to-have.

@Conduitry
Copy link
Member

Are never and always pretty much identical to using ignore and then putting a conditional redirect in handle before calling render? If so, I think those three options should suffice. If someone needs more control, they can use ignore and then put the appropriate redirects in their handle hook. Yes they may have to reimplement part of their routing logic there, but this feels obscure enough to me that it doesn't really deserve a special API when you can already do it manually.

@rmunn
Copy link
Contributor

rmunn commented May 17, 2021

Of these two semantics:

  • any index.svelte file gets a trailing slash if there are also non-index pages in the directory, or in child directories
  • the more deterministic alternative: all index.svelte files get a trailing slash (which dictates code organisation decisions, but is probably the decision we'd regret less)

I'd say "How about both?" I can see reasons why either one would be preferred for one person's use case vs. someone else's use case. The name "smart" feels like it belongs to the first one (where a decision is being made). For the second one, I'd propose "index", so that trailingSlash: "index" means "any index.svelte file gets a trailing slash" (i.e., the deterministic approach).

@rmunn
Copy link
Contributor

rmunn commented May 17, 2021

Ping @bdadam since he may want to express an opinion on the "smart" and "index" proposals in #1405 (comment)

@bencates
Copy link

bencates commented Jun 3, 2021

Would it be viable to allow this to be overridden on a per-route basis, like ssr/router/hydrate/prerender?

i.e. routes/admin/index.svelte file could look like this:

<script context="module">
  export const trailingSlash = 'always';
</script>

...and my routing table would then look something like

/
/about
/blog
/blog/1970/01/01/slug
/admin/  <= exception to the rule
/contact

where /admin/ and only /admin/ requires a trailing slash.

@rmunn
Copy link
Contributor

rmunn commented Sep 9, 2021

#2390 is a use case where a per-route override would make sense. There, the user wants to be able to set trailingSlash='always' except for the /manifest.webmanifest URL, where it needs to be never.

@Rich-Harris
Copy link
Member Author

Per-route is a little tricky, since waiting until the code has loaded to see if there's an override is terrible for performance. We could maybe add new options like "default:[always|ignore|never]" or trailingSlash: ['always', true] or something else that means 'load the page, and if it specifies an option use that, otherwise use the default', though for now the approach of using ignore alongside custom logic is probably the best workaround.

Noting that I said 'load the page' rather than 'load the route', since #4699 would mean that trailingSlash: 'always' only applies to pages, which I think is the behaviour we want.

@Rich-Harris
Copy link
Member Author

closing in favour of #7719

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

4 participants