Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Update <base> to reflect current route? #228

Open
Rich-Harris opened this issue Apr 3, 2018 · 15 comments
Open

Update <base> to reflect current route? #228

Rich-Harris opened this issue Apr 3, 2018 · 15 comments

Comments

@Rich-Harris
Copy link
Member

At present, <base> is the same for all pages, which means you can't use relative URLs. This was introduced for good reasons, but it turns out to cause problems.

I'm not sure that it can be removed without breaking base URL functionality though — for example, things like this would break:

<link rel=stylesheet href=global.css >
.thing {
  background-image: url(images/whatever.jpg);
}

Also, this.fetch(relativeUrl, ..) is tricky, because it needs to be relative to the page you're going to, not the page you're currently on, so it can't just straightforwardly delegate to fetch as it currently does.

It might be that the solution is just to not use relative URLs 😬 Unless there's already a creative solution for this problem?

@maxmilton
Copy link

maxmilton commented May 8, 2018

I would far prefer <base> removed completely and just have standard relative URLs. One big reason is same page links, e.g. <a href="#content">Skip to content</a>, currently redirect you to the index rather than just moving the viewport to the matching id="content".

As for referencing assets etc. why not do what we've always done and use a path relative to the site root:

<link rel=stylesheet href=/global.css>

Is there something with the routing that requires <base> to be set?

@aubergene
Copy link
Contributor

In my experience <base> usually adds more confusion than it solves. Could it use just root relative urls where it's needed, for stylesheet references and the like, they can be easily prefixed with the basepath when required. Then everything else can be relative. Just my two cents. I had a quick look at the routing in Sapper and it was too complex for me 😁

@Rich-Harris
Copy link
Member Author

Is there something with the routing that requires <base> to be set?

It's all to do with basepath. It's not uncommon to need to mount an app to a path other than root, and this was the easiest way I could find to make that possible. It is a nuisance though (for example, the other day I found that Safari and Firefox trip over url(#thing) inside SVGs, when they refer to local defs) so I'm definitely open to alternatives

@imbolc
Copy link

imbolc commented Oct 25, 2018

Why not substitute links during parsing to make it absolute? Eg

base_path = /foo/
app_path = /bar/
link: baz => /foo/bar/baz
link: /spam/ => /foo/spam

It won't solve fetch issue of course, I don't know if someone uses relative urls there. Maybe pass $path variable into each page for such case?

@kamiyaa
Copy link

kamiyaa commented Apr 16, 2020

Any updates on this?
I'm still having issues with same page links as described by @maxmilton

@thgh
Copy link
Contributor

thgh commented Apr 16, 2020

I would solve it with #984, perhaps someone can review?
It's backwards compatible so should be safe to merge in a minor version.

@arve0
Copy link

arve0 commented Apr 16, 2020

Also solved by #1152. A workaround while waiting for a solution in sapper is monkey-patching res.end:

// add this middleware before sapper.middleware
function removeBaseTag(req, res, next) {
    const resEnd = res.end;

    res.end = function () {
        const body = arguments[0];
        if (typeof body === "string") {
            arguments[0] = body.replace(/<base[^>]+>/, '');
        }
        resEnd.call(this, ...arguments);
    }

    next();
}

@thgh
Copy link
Contributor

thgh commented Apr 17, 2020

Also solved by #1152. A workaround while waiting for a solution in sapper is monkey-patching res.end:

Interesting approach, but that would break routes with a slash in it because styles are loaded from client/... instead of /client/...
I wish both PRs would get merged ^^

@arve0
Copy link

arve0 commented Apr 17, 2020

but that would break routes with a slash in it because styles are loaded from client/... instead of /client/...

Not sure if you mean this particular example or the pull request, but both could make links starting with / work. The example was made intentionally minimal, to show the technique.

If it is not clear, you would use the req-object to find the correct base/link-prefix for the current request, see req.path, etc. Then replace href/src-links (/href="\/[^"]"/). To avoid replacing links that already have correct base, check if start of link is equal to the base.

This is the naive approach, and might have corner cases I have not thought out, but I believe /-links should be straight forward. Links inside javascript that is dependent on concatenation is one corner case.

I wish both PRs would get merged ^^

A base true/false-setting would be easier than replacing. Replacements on the other hand fits more needs, as it's flexible. Until it gets decided, both, one or none, I'm comfortable to patch sapper with patch-package to have the proposed replacement API 🙂

@notpushkin
Copy link

Made a patch to save y'all trouble – based on @thgh's changes and can be installed with patch-package, like @arve0 suggested. Thank you guys!

@Conduitry
Copy link
Member

Are there any thoughts from people pushing for this (or the various related issues/PRs) about what hrefs should be relative to? Sapper currently allows page routes to be accessed with or without a trailing slash, which complicates the idea of relative links, and makes the whole thing messier. Even if we decided to normalize the URLs in some way (at least for the purpose of <base href> and resolving links, if not redirecting the actual pages), neither way of doing this (with or without trailing slash) is completely satisfactory, as each is more convenient in certain situations.

@notpushkin
Copy link

@Conduitry How about /foo.svelte/foo but /bar/index.svelte/bar/?

@imbolc
Copy link

imbolc commented May 2, 2020

neither way of doing this (with or without trailing slash) is completely satisfactory, as each is more convenient in certain situations

I won't call having two urls pointing to the same page a perfect solution either. The classic way is having a setting for normalization behaviour on application level. The approach suggested by @nolanlawson is more flexible, but in most cases I'd prefer the normalization to be uniform within an app.

Why can't we have all the options as an url_normalization setting:

  • default - for the current behaviour
  • add_slash - redirect /foo -> /foo/
  • remove_slash - redirect /foo/ -> /foo
  • notpushkin - /foo.svelte → /foo but /bar/index.svelte → /bar/

@aubergene
Copy link
Contributor

@Conduitry How about /foo.svelte/foo but /bar/index.svelte/bar/?

I'd be happy with this as a fix to #519. It's not about redirecting requests, that is the role of the web-server, it's about writing link hrefs to be going to the correct url in the first place

@dhrp
Copy link

dhrp commented Mar 4, 2021

I'd just like to add my ¢2: Having relative URL's will also greatly help with an internationalized site.

e.g. on a link like <a href="about"> on a page with path: /nl/ should take the user to /nl/about If links are not relative this means that you'll need to have the language in every single link.

Which is also a problem because for the purpose of DRY programming /about and /nl/about should be served by the same .svelte file and you would like to prevent moving the links into the translations files as much as possible.

Of course, the site developer will then need to make sure that path's are used correctly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

10 participants