Skip to content

fix: generate edge middleware to run reroute - #12296

Closed
teemingc wants to merge 99 commits into
mainfrom
fix-reroute-split
Closed

fix: generate edge middleware to run reroute#12296
teemingc wants to merge 99 commits into
mainfrom
fix-reroute-split

Conversation

@teemingc

@teemingc teemingc commented Jun 4, 2024

Copy link
Copy Markdown
Member

fixes #11879

This PR adds an edge middleware that runs reroute beforehand so that, when there are multiple functions, the correct one is invoked instead of not matching any function matchers.

The Vercel middleware uses the rewrite helper from @vercel/functions.

The Netlify middleware resolves the URL then passes it to the next handler.

EDIT: there's currently an issue with Vercel rewrites discarding the SvelteKit form action in the URL because the query parameter doesn't have a value vercel/vercel#12902

TODO

  • cleanup duplicated edge function generation code
  • docs
    - [ ] don't run reroute again in the split function that comes after the middleware
  • retain original URL via query params
    - apparently the Vercel edge middleware passes the original URL to the next function instead of the rewritten URL. So, we do need to run rewrite again after the middleware runs. In contrast, Netlify passes the rewritten URL back to the middleware then to the next function.
  • abstract implementation into an exported helper
  • netlify edge-functions-examples.netlify.app/example/rewrite
  • reliably import reroute hook from build output file when universal hooks file name is different

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.
    • no test for Vercel because I don't think there's a way to test the build output locally

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

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

@teemingc teemingc added the pkg:adapter-vercel Pertaining to the Vercel adapter label Jun 4, 2024
@changeset-bot

changeset-bot Bot commented Jun 4, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 0ce18bd

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

This PR includes changesets to release 3 packages
Name Type
@sveltejs/adapter-netlify Major
@sveltejs/adapter-vercel Major
@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

@teemingc teemingc changed the title fix: use Vercel edge middleware if reroute hook exists and more than one function fix: use Vercel edge middleware to run reroute Jun 4, 2024
Comment thread packages/adapter-vercel/files/reroute.js Outdated
@dominikg

dominikg commented Jun 4, 2024

Copy link
Copy Markdown
Contributor

Should this be a bit more generic so that it can be used by other adapters too? or is the way route splitting works so adapter-specific that they all need a separate implementation?

@teemingc

teemingc commented Jun 4, 2024

Copy link
Copy Markdown
Member Author

Should this be a bit more generic so that it can be used by other adapters too? or is the way route splitting works so adapter-specific that they all need a separate implementation?

I think it's possible for the part where we need to import the reroute hook and get the rewritten URL. The rest seems more platform specific. Vercel uses a custom header in a Response while Netlify just requires a URL https://edge-functions-examples.netlify.app/example/rewrite . Not sure about other platforms.

CC: @LorisSigrist

@LorisSigrist

LorisSigrist commented Jun 5, 2024

Copy link
Copy Markdown
Contributor

Each Serverless provider is going to have a different way of rewriting the URL so that would certainly be adapter-specific.

Given that .sveltekit/output/server/chunks/hooks.js already contains a bundled, useable version of the hooks.js file I don't see a reason to move the bundling outside of the adapter right now. The bundling in the adapter basically just amounts to concatonating the platform-specific code with an already bundled hooks.js file, which is unlikely to go wrong.

There is some fragility since we're assuming that the bundled file will always be at that path, meaning that changes in sveltekit's build-output may break the adapters, but I don't think that warrants adding a shared implementation right now. We can always share it later if the need arises.

@LorisSigrist

Copy link
Copy Markdown
Contributor

don't run reroute again in the split function that comes after the middleware

The most straight forward solution here would probably be to have a x-sveltekit-rerouted-from header to signal that the reroute hook does not need to run again & what the original URL was.

add abstraction for kit adapter API?

Let's wait on this one for now, this can be added later if the need arises from other adapters. When I suggested that we might need to extend the adapter API I didn't know we could just access files from the output-directory directly.

@teemingc

teemingc commented Jun 5, 2024

Copy link
Copy Markdown
Member Author

added a naive implementation to support Netlify then realised the adapter only supports split for normal functions. Couldn't even find an existing issue to add split support for edge functions. I guess the Netlify demand and support isn't that strong.

@teemingc

teemingc commented Jun 5, 2024

Copy link
Copy Markdown
Member Author

don't run reroute again in the split function that comes after the middleware

The most straight forward solution here would probably be to have a x-sveltekit-rerouted-from header to signal that the reroute hook does not need to run again & what the original URL was.

I wish we could do this but I'm not sure if it's possible on Netlify which only takes a URL object when rewriting and doesn't allow setting the headers like Vercel does (unless we decide to kick netlify to the curb and go ahead with the header implementation)

EDIT: maybe we can add the header in the serverless functions if split is enabled

@LorisSigrist

LorisSigrist commented Jun 6, 2024

Copy link
Copy Markdown
Contributor

maybe we can add the header in the serverless functions if split is enabled

That would be ideal

As a fallback we could go with a searchParam instead of a header, that should work everywhere.

@dominikg

dominikg commented Jun 6, 2024

Copy link
Copy Markdown
Contributor

what is the worst that would happen? that reroute is called again when the rerouted request hits the function?
If it is a fast sync function with stable result that shouldn't matter much, an early exit would be a small optimization.

Implementing it as a query param wouldn't really work unless the param is stripped again before it ever reaches the user. We must not mess with the presence or order in the querystring in any way, otherwise its going to break existing or future apps.

Even a custom header could be problematic in cases where headers are validated, but that also requires a change to the reroute signature. To introduce this in a non-breaking way would mean that you'll have to use return type like string | {url: string, more: boolean,stuff: string,here: any} and possibly also add new props to the input arg.

@LorisSigrist

Copy link
Copy Markdown
Contributor

Dominik is right

We have an error in our thinking. @eltigerchino and I assumed that the second reroute would be run on the already rerouted URL, but that's not actually the case. If it gets run again in the function then it gets run on the original URL and produce the same result. I just tested this in a deployment & it works as expected. We don't actually need to do the header/searchParam dance.

@teemingc

teemingc commented Jan 29, 2026

Copy link
Copy Markdown
Member Author

Going to resolve the merge conflicts then refactor the Vercel edge middleware to a normal serverless one https://vercel.com/docs/routing-middleware?framework=other . If we just fetch the response of the rerouted route, it should be the same as a rewrite without the current Vercel edge query param bug.

EDIT: the normal serverless middleware also uses x-middleware-rewrite so there's no difference

Comment on lines 52 to -65
@@ -60,9 +59,6 @@ export default async (request, context) => {
return server.respond(request, {
getClientAddress() {
return /** @type {string} */ (request.headers.get('x-forwarded-for'));
},
platform: {
context

@teemingc teemingc Feb 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

AFAIK context is no longer provided by Vercel? Or maybe they just don't document it anymore. I couldn't find this anywhere in https://vercel.com/docs/functions/runtimes/edge

@teemingc
teemingc marked this pull request as draft February 18, 2026 19:44
@teemingc

Copy link
Copy Markdown
Member Author

Converting to draft until we merge #15294 and maybe we wait for the environment API PRs

@teemingc teemingc added the needs-platform-tests This PR needs to run platform tests in order to merge. label Feb 25, 2026
@teemingc

Copy link
Copy Markdown
Member Author

Closing this in favour of #16590 which rebases this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-platform-tests This PR needs to run platform tests in order to merge. pkg:adapter-netlify pkg:adapter-vercel Pertaining to the Vercel adapter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

reroute hook breaks when deployed on Vercel if the app is deployed as multiple functions

7 participants