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

feat: set dynamic base when rendering page #9220

Merged
merged 23 commits into from
Feb 28, 2023
Merged

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Feb 26, 2023

Fixes #9162 by providing a fallback for the base path when global is unavailable

Follow-up to #9150. This sets base to a relative path when server-rendering, meaning that something like this...

<img src="{base}/images/potato.jpg" alt="a potato">

...will be rendered like so, depending on the page being rendered:

<img src="../../images/potato.jpg" alt="a potato">

When the element is hydrated, the attribute will change to reflect the actual base path:

<img src="/my-base-path/images/potato.jpg" alt="a potato">

This means that paths will work even if the site is running on the Internet Archive or somewhere. It needs some neatening up — ideally we wouldn't need to store original_base, and we also need to get assets working the same way — but you get the idea. Will finish this up tomorrow.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

@Rich-Harris Rich-Harris mentioned this pull request Feb 26, 2023
5 tasks
@Rich-Harris
Copy link
Member Author

This now works for both base and assets — SvelteKit now renders truly portable HTML. I did need to update a couple of tests though that were using base in a very specific way, so we need to decide whether this constitutes a breaking change.

@dummdidumm
Copy link
Member

"is this a breaking change or not" - tricky. I don't know how people use assets or base, but if they look at it to derive some logic from it, this would be a breaking change, and also surprising to them, since these are now relative but only during SSR. Regardless of how we decide, this should be documented on the options.

Will this have any weird implications on hydration (or possible future work in that area in Svelte core)? With this change, hydration changes the links.

@Rich-Harris
Copy link
Member Author

One solution to the breaking change question could be to make the behaviour opt-in:

export default {
  kit: {
    paths: {
      base: '.',
      assets: '.' // probably unnecessary to specify this, since `assets` defaults to `base`
    }
  }
};

That way if someone is doing something esoteric like styling internal links differently...

a[href^=/] {
  color: red;
}

...the behaviour won't change. We could swap the default round in 2.0.

One consideration around hydration is whether changing an href or a src would cause anything to reload (e.g. an iframe). I've tested it and it seems fine.

@benmccann
Copy link
Member

Could we combine the two changesets into one?

I hate to say it, but it does seem like this could be breaking

base: '.',

We should probably make it ./ to align with Vite: https://vitejs.dev/config/shared-options.html#base

@Rich-Harris
Copy link
Member Author

Yeah, I'm working on the base: '.' thing right now so that it's non-breaking.

./ doesn't make sense — base is used like ${base}/foo which should resolve to /foo or ./foo or /my-base/foo but never .//foo

@benmccann
Copy link
Member

I'd be curious why base: '.' turned out not to work, but this looks good to me

@Rich-Harris
Copy link
Member Author

Implementation-wise it was a clusterfuck. But it's also just confusing API, and prevents you from saying whether you want to use relative paths independently of whether there's a base path configured

@wighawag
Copy link
Contributor

wighawag commented Feb 28, 2023

Just tested with my app and it works, thanks so much for supporting this feature.

Edit: this was not correct:
It seems it support image root path too like <img src="/images/logo.svg" alt="logo"/> so we can use them wherever without needing a wrapper function.

It would be great if sveltekit could support the following when paths: {relative: true} is set

<img src="/images/logo.svg" alt="logo"/>
<a href="/about/">About page</a>

<link rel="icon" href="/pwa/favicon.ico" sizes="any" />
<link rel="apple-touch-icon" href="/pwa/apple-touch-icon.png" />
<link rel="manifest" href="/pwa/manifest.webmanifest" />

and transform those absolute path to relative

otherwise, we have to wrap the url into a function to make them use of import {base} from '$app/paths'; :

<img src={url('/images/logo.svg')} alt="logo" />
import { base } from '$app/paths';
export function url(p: string) {
	return `${base}${p}`;
}

@Rich-Harris
Copy link
Member Author

That's a separate feature request, and one that we've considered and refused: #8669 (comment)

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments. General observation (but not for this PR): render.js is becoming very big, maybe that method could benefit from some splits by concern so that if you're only looking for a specific thing in there you know you only have to look into method X - although this probably gets hard to do right since it's all concatenated into a string in the end.

packages/kit/src/core/config/options.js Outdated Show resolved Hide resolved

if (segments.length === 1 && paths.base !== '') {
// if we're on `/my-base-path`, relative links need to start `./my-base-path` rather than `.`
base = `./${paths.base.split('/').at(-1)}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why split.at(-1)? What if the base path spans more than one /, like /foo/bar? We know the base has to start with a slash, so why not

Suggested change
base = `./${paths.base.split('/').at(-1)}`;
base = `.${paths.base}`;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider a basepath like /a/b/c. If you're rendering that route, then {base}/d should resolve to /a/b/c/d.

With non-relative paths, that's easy — just use base as written. But if you're using a relative path, then it needs to be ./c to produce ./c/d to resolve to /a/b/c/d (or ../b/c/d, or ../../a/b/c/d, but there's no point in doing that).

With your suggestion, it would be ./a/b/c, which would resolve to /a/b/a/b/c/d

packages/kit/types/index.d.ts Show resolved Hide resolved
@wighawag
Copy link
Contributor

That's a separate feature request, and one that we've considered and refused: #8669 (comment)

In my opinion this is different as here no special path syntax would be required. Happy to open a new issue if there is a chance it could be re-examined in that context

@Rich-Harris
Copy link
Member Author

It's the same thing whether there's leading path syntax or not — API-wise it's highly confusing, and implementation-wise it's virtually impossible. You can use <base> for this.

Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
@Rich-Harris
Copy link
Member Author

render.js is becoming very big, maybe that method could benefit from some splits by concern

Agree, this has been on my wishlist for a while. I actually made a start on it recently, but scrapped it because it would have interfered with the PR in question — it should really be a pure refactor unattached to any other changes

Co-authored-by: Rich Harris <richard.a.harris@gmail.com>
@vnphanquang
Copy link
Contributor

Well done 🎉 🚀 . Thank you for adding this support 🙇🏼‍♂️. Previously I had to fork kit and hack around for this to work in one project that required so.

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.

Vitest fails after SvelteKit Update: Cannot find package '__sveltekit'
5 participants