Skip to content

feat: add reroute hook #11537

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

Merged
merged 49 commits into from
Jan 10, 2024
Merged

Conversation

LorisSigrist
Copy link
Contributor

@LorisSigrist LorisSigrist commented Jan 8, 2024

Closes #5703
Continuation of #11396

This PR adds a rewriteUrl hook that allows you to edit a url before SvelteKit uses it to render a route. This is different from a redirect, as it does not change the URL shown to the user, or change the value of event.url. Only the route resolution logic is affected.

Because this hooks is required both on the server and the client, this PR adds a new hooks.js file for isomorphic hooks. This can be changed if that's undesired.

API

//file: src/hooks.js

export const rewriteUrl = ({ url }) => url;

The returned URL get's used to determine which route to render.

Notes:

  • Mutating the URL argument doesn't do anything.
  • If the URL get's rewritten to an external URL, a 404 is always returned
  • The hook does not get applied to external URLs (important for client-side nav)
  • The hook is not applied recursively
  • If rewriteUrl throws an error on the client it falls back to a native navigation
  • If rewriteUrl throws an error on the server a 500 response is returned

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

Copy link

changeset-bot bot commented Jan 8, 2024

🦋 Changeset detected

Latest commit: 689811e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ollema
Copy link

ollema commented Jan 8, 2024

in the PR description:

//file: src/hooks.server.js

export const rewriteUrl = ({ url }) => url;

should this be:

//file: src/hooks.router.js

export const rewriteUrl = ({ url }) => url;

perhaps?

@LorisSigrist
Copy link
Contributor Author

When rewriteUrl throws an error, which behaviour would you expect?

  • Halt Server
  • 500 Response
  • Use original URL & log failure

@PatrickG
Copy link
Member

PatrickG commented Jan 8, 2024

When rewriteUrl throws an error, which behaviour would you expect?

* Halt Server
* 500 Response
* Use original URL & log failure

500 Response

@PatrickG
Copy link
Member

PatrickG commented Jan 8, 2024

I'm not sure about the name rewriteUrl.
All other hooks have handle in the name.

@Conduitry
Copy link
Member

Re the filename, I'm wondering whether just hooks.js might be better. Might we want more isomorphic hooks in the future? Is there a reason to separate out router hooks from other hypothetical isomorphic hooks?

@LorisSigrist
Copy link
Contributor Author

LorisSigrist commented Jan 8, 2024

Re the filename, I'm wondering whether just hooks.js might be better. Might we want more isomorphic hooks in the future? Is there a reason to separate out router hooks from other hypothetical isomorphic hooks?

I like hooks.js a lot, I'll change it to that!

I am a bit worried that this will cause some confusion with shared hooks. People might mistakenly believe that the isomorphic hooks file is where shared hooks can be added. The phrasing in the docs will need to be precise to avoid this.

Update: Changed it to just hooks.js

@LorisSigrist
Copy link
Contributor Author

I'm not sure about the name rewriteUrl. All other hooks have handle in the name.

I'm open to alternate name proposals, but I don't think having handle in all the hook names is really necessary. It doesn't really make it clearer that we are dealing with a hook, since that's already obvious from the filename. The function name just needs to be descriptive.

@PatrickG
Copy link
Member

PatrickG commented Jan 8, 2024

How about handleRouteMatch?
Instead of returning a URL, you have to return a MatchedRoute. If you do not return a MatchedRoute, it will result in a 404 Not Found.
With a matchRoute function as a param, that uses the sveltekit route matching (https://github.com/sveltejs/kit/blob/main/packages/kit/src/runtime/client/client.js#L345), which you can use if you don't want custom route matching (like the resolve param in handle).

type HandleRouteMatch = ({ url, routes, matchRoute }: { url: URL, routes: Route[], matchRoute: () => MatchedRoute }): MatchedRoute | null;

So matchRoute would be:

function matchRoute(routes: Route[], url: URL) {
  return routes.find((route) => route.exec(get_url_path(url.pathname)));
}

And the default hook would be

export function handleRouteMatch({ url, routes, matchRoute }) {
  return matchRoute(routes, url);
}

@LorisSigrist
Copy link
Contributor Author

I don't see the advantage of resolving a route instead of returning a url that then get's resolved to a route by the framework.
In order to do a rewrite with matchRoute, you already need to have edited the url in the exact same way that you would have if the hook just expected you to return a URL. In practice returning a route just adds some boilerplate around rewriteUrl.

// src/hooks.js
export function handleRouteMatch({ url, routes, matchRoute }) {
  return matchRoute(routes, rewriteUrl({ url }));
}

const mapping = {
  "/en/about" : "/en/about",
  "/de/ueber-uns": "/de/about",
}

function rewriteUrl({ url }) {
   url.pathname = mapping[url.pathname] ?? url.pathname
   return url
}

@PatrickG
Copy link
Member

PatrickG commented Jan 8, 2024

Yea, I just thought it might open this hook for other purposes.

@benmccann
Copy link
Member

Unless there's something that prevents you from doing it, we normally try to add new tests to existing test apps rather than adding a new test app. There's some amount of overhead for each new test app, so it helps keep the test suite from getting too slow if we can share. Hopefully you could put the new test in basics and just rewrite a handful of test URLs or something like that

@Rich-Harris Rich-Harris changed the title feat: add rewriteUrl hook feat: add reroute hook Jan 10, 2024
dummdidumm added a commit to sveltejs/language-tools that referenced this pull request Jan 10, 2024
Related to sveltejs/kit#11537
Also turns the error on unknown exports into a warning - people could be stuck on old versions of language tools while new features are added, and SvelteKit will throw a dev time error anyway
@Rich-Harris Rich-Harris merged commit 4adbce0 into sveltejs:main Jan 10, 2024
@Rich-Harris
Copy link
Member

thank you!

@vnphanquang
Copy link
Contributor

vnphanquang commented Jan 11, 2024

Hello everyone. Thanks for the good work. Given the example usage in the docs

/** @type {Record<string, string>} */
const translated = {
	'/en/about': '/en/about',
	'/de/ueber-uns': '/de/about',
	'/fr/a-propos': '/fr/about',
};

/** @type {import('@sveltejs/kit').Reroute} */
export function reroute({ url }) {
	if (url.pathname in translated) {
		return translated[url.pathname];
	}
}

/de/about and /fr/about ares still valid end-user routes (i meant user can still go directly to /de/about, for example). I'm not sure I like this. If one decides to localize slugs, they wouldn't want to have duplicated paths yeah? To make this SEO friendly there needs to be extra redirect setup. Any thoughts?

---UPDATE---

I've tried a working redirect setup in hook.server like so:

import { redirect } from '@sveltejs/kit';

/** @type {import('@sveltejs/kit').Handle} */
export async function handle({ event, resolve }) {
	if (event.url.pathname === '/de/about') {
		redirect(301, '/de/ueber-uns')
	} else if (event.url.pathname === '/fr/about') {
		redirect(301, '/de/a-propos')
	}
	return resolve(event);
}

Without reading kit source code, this tells me reroute runs after handle, is that correct? I think devs would benefit if some clarification is mentioned in the docs.

@LorisSigrist
Copy link
Contributor Author

Good Point!
Just to clarify, reroute runs before handle, since it's needed to determine event.route.id

There are two ways I can think of to address this:

  1. Enforce the correct translation in the handle hook like you've done
  2. Omit the language in the rerouted path and reroute to /about directly. The language can still be read from the URL.

@vnphanquang
Copy link
Contributor

vnphanquang commented Jan 11, 2024

Thank you @LorisSigrist. (2) does seem more convenient, however, we still need to redirect the /about url, assuming we only want to support /<lang>/about


Just to clarify, reroute runs before handle, since it's needed to determine event.route.id

I see, thanks for clarifying. It is certainly something I struggle to wrap my head around at first, perhaps due to the way reroute example code is presented, which suggested, in my mind, a redirect (i know it is not). I think the distinction between a reroute and a redirect should be emphasized in the docs as well. Here is my understanding now if we combine both reroute and redirect from handle:

re-direct-route

@LorisSigrist
Copy link
Contributor Author

LorisSigrist commented Jan 11, 2024

Yes, you would still need to redirect /about from inside the handle hook.

Your understanding diagram looks mostly right, except that route.id doesn't exist yet in reroute. Reroute only has access to the url and nothing else.

@Rich-Harris
Copy link
Member

Reroute only has access to the url and nothing else.

(Per #11595, it might be desirable to have access to cookies etc — perhaps we need to create the event before reroute, just without route and params. Something for future us to consider)

dummdidumm added a commit to sveltejs/language-tools that referenced this pull request Jan 11, 2024
Related to sveltejs/kit#11537
Also turns the error on unknown exports into a warning - people could be stuck on old versions of language tools while new features are added, and SvelteKit will throw a dev time error anyway. Also commented out that never worked.
@aeons
Copy link
Contributor

aeons commented Feb 8, 2024

This PR changes the generated get_hooks to import the hooks asynchronously, which means that "runs-once-at-startup" code in either hooks file is not run before the first request is received.

Is there somewhere else this type of code can be placed?

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.

Support for rewrites
9 participants