kit
kit copied to clipboard
fix: generate edge middleware to run `reroute`
fixes https://github.com/sveltejs/kit/issues/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/edge.
rewritedocs - https://vercel.com/docs/functions/edge-middleware/middleware-api#rewritesrewriteimplementation - https://github.com/vercel/vercel/blob/cefda60a603d60cc35e4697c36e751cca411e6bb/packages/functions/src/middleware.ts#L101- edge middleware example - https://github.com/vercel/examples/tree/main/build-output-api/edge-middleware
The Netlify middleware simply requires returning the new URL.
- Netlify rewrite example - https://edge-functions-examples.netlify.app/example/rewrite
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 https://github.com/vercel/vercel/issues/12902
TODO
- [x] cleanup duplicated edge function generation code
- [x] docs
~- [ ] don't run
rerouteagain in the split function that comes after the middleware~ - [x] 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.~~
- [x] abstract implementation into an exported helper
- [x] netlify edge-functions-examples.netlify.app/example/rewrite
- [x] 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:
- [x] 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
- [x] This message body should clearly illustrate what problems it solves.
- [x] 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
- [x] Run the tests with
pnpm testand lint the project withpnpm lintandpnpm check
Changesets
- [x] If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.
Edits
- [x] Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.
🦋 Changeset detected
Latest commit: c8c9616559fb26e8a206ee6a6ebc2c39b4c94207
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
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?
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
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.
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.
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.
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-fromheader to signal that thereroutehook 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
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.
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.
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.
Awesome. What's left is a mention in the vercel and netlify adapter docs and some cleanup for those duplicated edge function generators. Unless it's okay to leave them separate? They have their similarities but also some differences that might be hard to reconcile
Two things I'm unsure of:
- Should this be a be a minor or a patch bump? It is a fix so that
rerouteworks correctly with split functions but people should know that they'll start getting billed for edge requests since it runs in an edge middleware. - Should there be more details added to the documentation? Such as the reason why it needs to be in an edge middleware or that
reroutewill run twice: once in the edge middleware and once in the route handler function?
Should this be a be a minor of patch bump? It is a fix so that reroute works correctly with split functions but people should know that they'll start getting billed for edge requests since it runs in an edge middleware.
I think the most important thing to do here might be to split the changeset into two: one for netlify and one for vercel. Then in the vercel changeset we can mention the possible billing impact. If we call it a new feature and make it a minor it might slightly increase the chances that people will view the changelog when upgrading, so I suppose it'd be reasonable to do that.
Should there be more details added to the documentation? Such as the reason why it needs to be in an edge middleware or that reroute will run twice: once in the edge middleware and once in the route handler function?
I'm afraid I haven't kept up with all the discussion in this PR. Can you provide a quick summary of why that is? I can try to provide an opinion on whether/how to add it to the docs after I understand it better
Should there be more details added to the documentation? Such as the reason why it needs to be in an edge middleware or that reroute will run twice: once in the edge middleware and once in the route handler function?
I'm afraid I haven't kept up with all the discussion in this PR. Can you provide a quick summary of why that is? I can try to provide an opinion on whether/how to add it to the docs after I understand it better
Why it runs in an edge middleware
Vercel and Netlify both invoke serverless functions based on the request URL but the reroute hook only runs after a serverless function is invoked. If we run reroute before the deployment platform's routing, it will be able to invoke the correct serverless function (instead of the fallback that displays a 404).
Why does it run twice?
It runs once in the middleware and once when the serverless function is invoked because of the previous reason. So we need a way to disable it when the server function runs if it has already been delegated to an edge middleware. One suggestion was to set a flag in the request headers, but Netlify's edge middleware only allows modifying either the URL or headers at a given time, not both. (maybe we set the header flag then modify the URL in a second middleware/passthrough?)
But now thinking about it, maybe we could also add a conditional to not run it if the adapter.config.split is set to true? (that's an adapter option though...)
Should this be a be a minor of patch bump? It is a fix so that reroute works correctly with split functions but people should know that they'll start getting billed for edge requests since it runs in an edge middleware.
I think the most important thing to do here might be to split the changeset into two: one for netlify and one for vercel. Then in the vercel changeset we can mention the possible billing impact. If we call it a new feature and make it a minor it might slightly increase the chances that people will view the changelog when upgrading, so I suppose it'd be reasonable to do that.
How about just changing it to a minor for both since the Netlify adapter would also have an impact on billing. Would we still need to split the changeset then?
- Netlify pricing. Edge middleware is the same as an edge function.
- Vercel pricing. Edge middleware is different and cheaper than Vercel function pricing.
I've updated the changesets to a minor so that it's more likely people will read it and the new billing impact.
Looking forward to this! My current situation with a NextJS rewriter in front of my SvelteKit app on Vercel is brittle and causing a lot of wrong-URL headaches. Thanks for the work on it @eltigerchino
preview: https://svelte-dev-git-preview-kit-12296-svelte.vercel.app/
this is an automated message
I've run into a hurdle and can't figure out how to get esbuild to recognise @vercel/edge as a dependency to bundle. Is the only option for users need to manually install this? Is there no way to get it to resolve to the dependency installed by the Vercel adapter?
EDIT: it seems like the Netlify adapter bundles the file using rollup in a prepublish step so I'll try that here
Updated the PR to include the same reroute fixes from https://github.com/sveltejs/kit/pull/13092 in the Vercel and Netlify middleware functions
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 https://github.com/vercel/vercel/issues/12902
~~EDIT: the issue has been resolved~~
Preview: https://svelte-dev-git-preview-kit-12296-svelte.vercel.app/