sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
17.97k stars 1.81k forks source link

trailing slash removed when navigating #733

Closed wighawag closed 3 years ago

wighawag commented 3 years ago

Describe the bug I would like my path to end up with a trailing slash. Currently with these lines, svelte kit always remove it : https://github.com/sveltejs/kit/blob/84e9023c06dc3579972f93e0d5cf1f2481aa3be4/packages/kit/src/runtime/client/router.js#L185-L188

Expected behavior This behavior should at least be configurable.

Having a trailing slash is important to me as ipfs gateways append it anyway and so without being able to keep the trailing slash, there is a different behaviour between SPA navigation and navigation through request.

Plus it cause issue to calculate relative path as without slash the resource is now considered to be in the parent folder

I think trailing slash are more accurate in that regard since on a static website, the corresponding underlying index.html file is in its own folder.

Severity This is a blocker for me that deploy website on ipfs

I am working on a ipfs adapter to support ipfs properly and I am currently stuck with this.

wighawag commented 3 years ago

The adapater repo is here : https://github.com/wighawag/sveltejs-adapater-ipfs See readme for what it actually does : https://github.com/wighawag/sveltejs-adapater-ipfs/blob/main/README.md

The test app is here : (branch : adpater-ipfs) can be found here : https://github.com/wighawag/svelte-kit-ipfs-example/tree/adapter-ipfs

With the current modif it works but ideally I would not need to replace generated code as this is brittle

Rich-Harris commented 3 years ago

previous discussion here, which lead to #267. from there:

Starting with no slashes for the beta and waiting until we get a use case for something else sounds like a sensible plan to me

Sounds like we have a use case!

What we don't have is a sense of how this should be configured. Is an app-wide setting sufficient, or does it need to be per-page? What sort of thing did you have in mind?

wighawag commented 3 years ago

Hi @Rich-Harris thanks for the link to past discussion

What we don't have is a sense of how this should be configured. Is an app-wide setting sufficient, or does it need to be per-page?

For my use case (ipfs hosting), an app-wide config would be sufficient

ipfs gateway force the trailing slash when an index.html is detected and used.

The trailing slash make it easier to compute relative path too

mcmxcdev commented 3 years ago

Another use case is SEO, the same route with slash and without would be indexed as two different pages by a crawlbot, therefore deciding to redirect to either or makes sense.

techniq commented 3 years ago

FWIW regarding relative paths, I borrowed/tweaked this url building from routify's helpers to better handle them

import { derived, get } from 'svelte/store';
import { page } from '$app/stores';
import { goto as gotoApp } from '$app/navigation';

export function url(path: string) {
  const $page = get(page);

  if (path.match(/^\.\.?\//)) {
    // Relative path (starts with `./` or `../`)
    let [, breadcrumbs, relativePath] = path.match(/^([\.\/]+)(.*)/);
    let dir = $page.path.replace(/\/$/, '');
    const traverse = breadcrumbs.match(/\.\.\//g) || [];
    // if this is a page, we want to traverse one step back to its folder
    traverse.forEach(() => (dir = dir.replace(/\/[^\/]+\/?$/, '')));
    path = `${dir}/${relativePath}`.replace(/\/$/, '');
    path = path || '/'; // empty means root
    console.groupEnd();
  } else if (path.match(/^\//)) {
    // Absolute path (starts with `/`)
    return path;
  } else {
    // Unknown
    return path;
  }

  return path;
}

export function goto(path: string) {
  const newPath = url(path);
  return gotoApp(newPath);
}

Examples:

<a href={url(`./${item.Id}`)}>Child / Subitem path</a>
<a href={url(`../${key}`)}>Sibling path</a>
<a href={url('../')}>Parent path (back to list, etc)</a>
<a href={url('../../')}>Grandparent path</a>
Rich-Harris commented 3 years ago

What's wrong with just using relative paths?

techniq commented 3 years ago

@Rich-Harris it's been a little while to remember all the details, but when I tried to use URL I remember it not being as versatile as I needed.

For example, if I want to add a child/sub path (tack on a /id to the path), neither id nor ./id would do this (at least how I expected)

new URL('id', 'http://example.com/one/two/three').pathname
=> "/one/two/id"
new URL('./id', 'http://example.com/one/two/three').pathname
=> "/one/two/id"

I think this is mostly due to the path not ending in a / (which I prefer it doesn't) but that is also why I mentioned it here.

There are more use cases I have such as those listed above (replace the current last path (sibling path), parent path, etc). Some of those work with URL and relative path, but not all. Don't rule out that I could just be doing it wrong, but when I researched back then (stackoverflow, etc), it didn't seem possible and hence finding how routify was doing it (since I was using it before).

vyckes commented 3 years ago

Another use case to to not remove the trailing spaces, is how Netlify handles URLs by default. URLs without a trailing spaces will automatically get redirected (301) to the one with a trailing slash, as you can see in the below screenshot. This is impacting performance a little bit, which I measured with web.dev.

CleanShot 2021-04-07 at 11 12 21@2x

You can change it by updating the asset deployment settings of Netlify (disable the prettifier for URLs), but it is not clear this is the solution to this issue, and it (potentially) impacts other URLs in a negative way.

FYI, I am using the static adapter to deploy to Netlify.

RaeesBhatti commented 3 years ago

Netlify redirects tries the exact path in the request and if that is not available it will redirect to trailing slash or non-trailing slash whichever one is available.

RaeesBhatti commented 3 years ago

I've been looking into SvelteKit routing deeper recently for https://github.com/sveltejs/kit/pull/943. And I think I understand some of the reasoning behind the current design. Making routes/category/index.svelte responsible for /category achieves the following:

Because there's no other way to scope those two features down to a specific path if we're using filesystem based routing.

The trailing slash and non-trailing slash URLs can exist at the same time and support existing features in a couple of different scenarios. E.g.

  1. Using $index.svelte file for trailing slash:

    • routes/category/index.svelte/category
    • routes/category/$index.svelte/category/

    The problem with this approach is that /category and /category/ can't have different hooks and layouts.

  2. Using $index directory for trailing slash

    • routes/category/index.svelte/category
    • routes/category/$index/index.svelte/category/

    This might be a little confusing at first glance but allows scoping of layout and hooks.

  3. Using $ directory for trailing slash

    • routes/category/index.svelte/category
    • routes/$category/index.svelte/category/

    I think this probably the cleanest. We could also go with # as prefix for directory name.

I think any of these can be implemented without breaking existing behavior. I'm willing to put in the work and implement this since we have a need for this kind of routing for a website we want to port to SlvelteKit. But I'd like to get a go ahead from SvelteKit authors and maintainers first.

wighawag commented 3 years ago

@RaeesBhatti It is an interesting approach to allow for both.

I still think we need a way to have that configurable globally though and let user use normal folder name if desired. This is especially important for ipfs gateway which do not allow for path that do not ends with slash. They get redirected to path with a trailing slash. It would be confusing to force user to use $foldername.

Your proposal should be an opt-in option that let users use both trailing slash and non-trailing slash if they need it, so that use of trailing slash can continue to use the normal folder name.

RaeesBhatti commented 3 years ago

@Conduitry could you chime in with your opinion on this!

I personally prefer option 3. It can be documented in the routing section to quell any confusion.

WaltzingPenguin commented 3 years ago

Another vote and use case for not stripping trailing slashes: write a blog inside of SvelteKit, use MDsvex for posts, and attempt to link to a resource in the same directory.

Write the post /my-first-article/index.svx. Reference a file inside the same directory via ![useful-alt-text](./my-image.png) and the link will be broken. You are instead forced to write ![useful-alt-text](./my-first-article/my-image.png) and hope the directory never gets renamed/moved. I know referencing images via imports is generally preferred, but that's not really ideal for content authoring and vite handles img tags just fine on its own.

benmccann commented 3 years ago

This may be a more serious issue. It turns out that Netlify redirects to the trailing slash version and SvelteKit redirects to the version without trailing slash so you get an infinite redirect: https://github.com/sveltejs/kit/issues/1346

peterbabic commented 3 years ago

Is it connected to the Pretty URLs feature discussed in the netfify guide?

mcmxcdev commented 3 years ago

I actually have all the optimizations by Netlify disabled, so I would say not related to that.

bjon commented 3 years ago

Does SvelteKit really have to care about slashes at all (sapper doesn't)? This is a blocker for us, cause our site has been in production for 2 years. We don't want to change all canonical paths, sitemaps, and reindex all search engines.

We're using a hybrid atm, /[lang]/path

This should probably be mentioned at the top of the migration guide.

peterbabic commented 3 years ago

@bjon Sapper has an open issue of https://github.com/sveltejs/sapper/issues/519 and it states not that it does not care, but that it inconsistent and it is confirmed at the end with multiple 👍🏻 reactions that this behavior in fact affects people. Speaking for myself, I would love to have it in kit consistently without a trailing slash by default, with the possibility of an individual override.

bjon commented 3 years ago

I don't really care how its done, as long as its possible to migrate from sapper :)

bdadam commented 3 years ago

To me the most straightforward way to do the routing seems to be the following:

I.e. the last index.svelte is always replaced by a trailing slash. It is the same logic how all webservers treat index.html.

Rich-Harris commented 3 years ago

I've gone back and forth on @bdadam's suggestion — there is a pleasing simplicity to it, but I don't love the fact that path semantics are dictated by what are essentially code organisation decisions.

An alternative would be to add the trailing slash if and only if there's a non-index page in the directory as well — i.e. foo/index.svelte by itself is /foo, but if there's a foo/bar.svelte then it's /foo/. Arguably that should be opt-in though, since it might be surprising if the behaviour of one page is determined by the presence or absence of another.

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.)

Then again, those cases would be rare, so we might be willing to live with that failure case.

One possibility that should cover all the bases would be to have an app-wide trailingSlash config option with the following possible values:

Thoughts? We could also start with just never, always and ignore, if we're unsure about smart.

bjon commented 3 years ago

@Rich-Harris that would work great for us

benmccann commented 3 years ago

I agree with having never be the default. always makes sense as well. I'm not convinced on the need for ignore, but not against it either as it seems reasonable.

The smart mode doesn't make any sense to me because it seems really inconsistent. I actually had no idea that Apache or whatever would do that. I guess it's a holdover of trying to treat things as files vs directories, but it's not actually meaningful on the web. As a user of a website I have no idea what directory layout it uses nor should I and so I don't see why I'd want that extra information communicated to me in a URL especially since 99.9% of users won't understand that subtly. I don't think we should implement it just because another piece of software does especially given the extra complication it produces. I can't think of any reason you'd want to do it on a new project. The only reason I can think of to do it on an old project is if you're migrating from Apache, etc. and want the URLs to remain exactly the same for SEO purposes though I just researched this and it appears not to be a concern (Google would previously drop 15% of page rank going through a 301 redirect, but changed it a number of years ago so that there's no penalty), so even on migrations it doesn't make sense to me.

WaltzingPenguin commented 3 years ago

I've gone back and forth on @bdadam's suggestion — there is a pleasing simplicity to it, but I don't love the fact that path semantics are dictated by what are essentially code organisation decisions.

But that's true for the entirety of file based routing. I would love to have login, registration, etc. in their own folder named auth but, if I want them to have the url of "/login" or "/registration", they have to live in the root folder. This is the compromise for not having to manually declare routes.

@bdadam's suggestion is not only easy to teach and implement, but it is also the only one that is deterministic. Under that scheme, I can look at the current URL and tell you exactly which file path to check for the entry point. For every other proposed scheme, I know the general ballpark to look but I will have to look at the filesystem to determine exactly which file to open.

It does also allow both /about and /about/ to exist. They are two different resources and shouldn't be treated as interchangeable. I know I've run into issues with Sapper previously because it thought it knew my paths better than I did.

I agree with having never be the default. always makes sense as well. I'm not convinced on the need for ignore, but not against it either as it seems reasonable.

The argument for ignore is simply that SvelteKit is the wrong layer to fix things. Redirects are better handled at the server/hosting level (be that ngnix or Netlify).

For internal links in a project, /about/ should result in an 404 if the correct URL is /about. The error will get caught and fix during development. If both /about/ and /about resolve to the same resource, there will be all kinds of wrinkles introduced as the code moves to different environments.

Rich-Harris commented 3 years ago

It does also allow both /about and /about/ to exist. They are two different resources and shouldn't be treated as interchangeable. I know I've run into issues with Sapper previously because it thought it knew my paths better than I did.

Interesting — I would have regarded that as a bug, not a feature. The fact that they're technically separate URLs has always felt like an accident of history; I can't imagine a situation in which you'd want to have /about and /about/ in the same app. Is there a reason to have that?

@bdadam's suggestion is not only easy to teach and implement, but it is also the only one that is deterministic. Under that scheme, I can look at the current URL and tell you exactly which file path to check for the entry point. For every other proposed scheme, I know the general ballpark to look but I will have to look at the filesystem to determine exactly which file to open.

This is true (up to a point — I don't know that /blog/some-article is driven by src/routes/blog/[slug].svelte without looking at the filesystem), but the determinism comes at a price over and above the lost code organisation flexibility, since it only works if we don't do any redirecting, and that means that users have to be careful when manually typing URLs.

Also, it introduces a wrinkle with prerendering: at present, src/routes/blog.svelte and src/routes/blog/index.svelte both result in blog/index.html being prerendered, because we don't assume that blog.html will be served for requests to /blog (and we can't skip the extension because then the file and the directory of child pages/data would have the same name, irrespective of MIME type confusion). That isn't set in stone, of course, but it's a new set of things to think about.

The smart mode doesn't make any sense to me because it seems really inconsistent. I actually had no idea that Apache or whatever would do that. I guess it's a holdover of trying to treat things as files vs directories, but it's not actually meaningful on the web.

The benefit isn't to the user, it's to the developer. It means that your src/routes/blog/index.svelte can have links like this...

<a href="./{post.slug}">{post.title}</a>

...rather than this (which is less portable — you can't just rename the directory 'articles' or trivially handle localised routes):

<a href="/blog/{post.slug}">{post.title}</a>
Rich-Harris commented 3 years ago

Opened a separate issue to discuss "smart" and "strict", since this issue would be closed by #1404

mcmxcdev commented 3 years ago

In case someone else also has to figure this out: trailingSlash: "always" leads to every page going into infinite loop with Netlify. trailingSlash: "ignore" works for all pages and lets Netlify handle the trailing slashes. <- this is what you want.

janosh commented 3 years ago

trailingSlash: "ignore" works for all pages and lets Netlify handle the trailing slashes. <- this is what you want.

@mhatvan Are you referring to the "Pretty URLs" option under Deploy Settings > Asset optimization?

Screen Shot 2021-05-12 at 06 49 27

Mlocik97 commented 3 years ago

Interesting — I would have regarded that as a bug, not a feature. The fact that they're technically separate URLs has always felt like an accident of history; I can't imagine a situation in which you'd want to have /about and /about/ in the same app. Is there a reason to have that?

file browsing in browser (for example for cloud storages or repos, or torrent trackers),... if it's ending with symbol / it's folder, if not, it's file. That's used pretty often this way.

benmccann commented 3 years ago

file browsing in browser (for example for cloud storages or repos, or torrent trackers),... if it's ending with symbol / it's folder, if not, it's file. That's used pretty often this way.

But in that case you're not going to have a new .svelte file for each file or directory. You're going to have a single .svelte file that acts as the file browser. That .svelte file can dynamically show different contents based on the URL including whether it has a slash or not. You wouldn't need any support in SvelteKit's file system router for that

bdadam commented 3 years ago

@Rich-Harris Thanks for implementing trailingSlash: "never" | "always" | "ignore". I was wondering if it could somehow be possible to set this flag on a page basis? I imagine there are quite some websites which haven't implemented a consistent trailing slash strategy and just use a mix of everything.

I could imagine either exporting it in <script context="module"> OR trailingSlash in svelte.config.js could be a function which takes the path and returns true or false.

trailingSlash: (path) => true

OR

<script context="module">
    export const trailingSlash = true;

    export async function load({ page, fetch }) {
        return {
            props: {
                // ...
            },
        };
    }
</script>

What do you think?

peterbabic commented 3 years ago

@bdadam is it entirely necessary? I believe having an inconsistent behavior here here is an anti-pattern. Also with trailingSlash: ignore you can probably let your webserver handle all the required behavior by the rules you define.

Although SvelteKit is aiming to support services like Vercel or Netlify, where the webserver config is not available, there are probably many other uses for the Kit that might not even involve aforementioned services (for instance waiting to be able to finally use things like https://github.com/svelte-add/tailwindcss in production env), but maybe I am wrong here.

Mlocik97 commented 3 years ago

file browsing in browser (for example for cloud storages or repos, or torrent trackers),... if it's ending with symbol / it's folder, if not, it's file. That's used pretty often this way.

But in that case you're not going to have a new .svelte file for each file or directory. You're going to have a single .svelte file that acts as the file browser. That .svelte file can dynamically show different contents based on the URL including whether it has a slash or not. You wouldn't need any support in SvelteKit's file system router for that

Agree, if we are talking about routes/*.svelte files,... but what if we are talking about routes/*.js (endpoint) files, when programmer write logic in it, that if there is "/" at the end of URL, it will provide content of folder, but if not, then content of file?

benmccann commented 3 years ago

Agree, if we are talking about routes/.svelte files,... but what if we are talking about routes/.js (endpoint) files, when programmer write logic in it, that if there is "/" at the end of URL, it will provide content of folder, but if not, then content of file?

You can still do that today in SvelteKit it you'd like

bdadam commented 3 years ago

I just filed an issue in the static adapter which IMHO would be easier to tackle if the file path determined what the output filename is, e.g. routes/hello.svelte => hello.html, routes/hello.html.svelte => hello.html.html, routes/hello/index.svelte => hello/index.html.

eikaramba commented 2 years ago

FYI: relative paths within a file like /xyz/index.svelte pointing to /subfolder/123 do not translate to /xyz/subfolder/123 for me (using ordinary links). very weird and not what i was expecting. https://github.com/sveltejs/kit/issues/733#issuecomment-810177995 solution did however solve it. just leaving this here as i am not sure if this is a bug or just how svelte works.

joetm commented 2 years ago

Use case: Hosting on AWS S3.