-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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: middleware for several adapters #13477
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: f9efcbd The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
@@ -37,6 +37,16 @@ const get_default_runtime = () => { | |||
// https://vercel.com/docs/functions/edge-functions/edge-runtime#compatible-node.js-modules | |||
const compatible_node_modules = ['async_hooks', 'events', 'buffer', 'assert', 'util']; | |||
|
|||
const [major, minor] = VERSION.split('.').map(Number); | |||
const can_use_middleware = major > 2 || (major === 2 && minor > 17); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is currently always false
because we're not at 2.18 yet (we will if this is released); so if you want to test this out you gotta adjust this.
I'm going to try this again here, but could middleware somehow be leveraged to bypass CSRF protection for individual routes? See #6784. |
No that would not help there, because the CSRF checks happen within the SvelteKit runtime. For now you have to go with the workaround mentioned in the issue of disabling checks altogether and reimplement them within the handle hook. |
…ave to care about it
…endpoints which allows SvelteKit of handling the difference of imports in dev/preview
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few notes on adapter-node docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adapter-cloudflare
docs notes
documentation/docs/25-build-and-deploy/60-adapter-cloudflare.md
Outdated
Show resolved
Hide resolved
documentation/docs/25-build-and-deploy/60-adapter-cloudflare.md
Outdated
Show resolved
Hide resolved
documentation/docs/25-build-and-deploy/60-adapter-cloudflare.md
Outdated
Show resolved
Hide resolved
documentation/docs/25-build-and-deploy/60-adapter-cloudflare.md
Outdated
Show resolved
Hide resolved
documentation/docs/25-build-and-deploy/60-adapter-cloudflare.md
Outdated
Show resolved
Hide resolved
|
||
The middleware runs on all requests that your worker is invoked for, which is dependent on the [`include/exlcude` options](#Options-routes). | ||
|
||
> [!NOTE] Locally during dev and preview this only approximates the capabilities of middleware. Notably, you cannot read the request or response body, and middleware runs on all requests except those that would end up in `_app/immutable`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stuff about _app/immutable
doesn't appear to be accurate? It does run on _app/immutable
in preview
. For dev
I realise that it's not running on files that are being transformed by Vite, though IIUC that's not adapter-specific, and I'm not totally sure I understand what we're excluding and why — will try and get my head round this
> [!NOTE] Locally during dev and preview this only approximates the capabilities of middleware. Notably, you cannot read the request or response body, and middleware runs on all requests except those that would end up in `_app/immutable`. | |
> [!NOTE] During `dev` and `preview` you cannot read the request or response body. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For dev I realise that it's not running on files that are being transformed by Vite, though IIUC that's not adapter-specific, and I'm not totally sure I understand what we're excluding and why
It's excluding everything that looks like unbundled files that will not ever be requested like that during production (because it's bundled and the path will be totally different). In other words everything that would end up in _app/immutable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this pairs with a separate concern I have, which is that someone might expect to be able to do this sort of thing...
const response = await next();
return new Response(response.body!.pipeThrough(transform), response);
...which is currently prohibited in these middlewares. If it wasn't, then I'd expect to be able to transform source code at runtime whether in dev or prod. (I feel like it has to be possible, I have distinct memories of monkey-patching res.write
and res.end
before calling next()
in Express apps of yore.)
Perhaps that's an unreasonable expectation, though if we're not going to expose the full capabilities of the platform then I find myself wondering anew about lowest-common-denominator cross-platform APIs...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, we are exposing them, but not at dev/preview time.
Is it realistic that someone would do these things in a SvelteKit app. Feels like something you'd do in the handle hook.
So how to proceed here? Investigate if this is possible to replicate? And go with the lowest common denominator after all if not? Or just do that right away? I get the feeling that you're not really happy with this uncanny valley after looking more closely.
What if instead of |
I'm fine either way but additionalEntryPoints to me signals more that this is something on top, not something that replaces something else |
packages/adapter-cloudflare/index.js
Outdated
// We omit the body here because it would consume the stream | ||
req.method === 'GET' || req.method === 'HEAD' || !req.headers['content-type'] | ||
? undefined | ||
: 'Cannot read body in dev mode' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels like this will cause weird bugs — we might be better off with something more forceful?
if (req.method !== 'GET' && req.method !== 'HEAD') {
Object.defineProperty(request, 'body', {
get() {
throw new Error('Cannot read request body in dev/preview');
}
});
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we should - though I opted for a warning only, because in preview you have no way to know that you're not in real prod mode (in dev you can use import { dev } from '$app/environment'
), so you'd have no other choice but to wrap everything in try-catch which feels nasty.
@@ -107,6 +107,53 @@ Cloudflare Workers specific values in the `platform` property are emulated durin | |||
|
|||
For testing the build, you should use [Wrangler](https://developers.cloudflare.com/workers/wrangler/) **version 3**. Once you have built your site, run `wrangler pages dev .svelte-kit/cloudflare`. | |||
|
|||
## Pages Middleware | |||
|
|||
You can deploy one middleware function that closely follows the [Pages Middleware API](https://developers.cloudflare.com/pages/functions/middleware/). Unlike the [handle](/docs/kit/hooks#Server-hooks-handle) hook, middleware runs on all requests, including for static assets and prerendered pages (depending on your configuration). If using [server-side route resolution](configuration#router) this means it runs prior to all navigations, no matter client- or server-side. This allows you to for example run A/B-tests on prerendered pages by rerouting a user to either variant A or B depending on a cookie. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
closely follows
In real Pages middleware you can export onRequestGet
and onRequestPost
and so on, and the exports can be arrays of middlewares. How committed are we to making this resemble the real thing?
I feel like we should either aim for full fidelity or avoid making it look like something it's not — I'm getting a real uncanny valley sensation to be honest
I think it's okay to distinguish between framework-provided entry points and adapter-specified entry points; I don't feel like any clarity is lost personally. Seven syllables is a lot! |
}; | ||
/** | ||
* Creates an `Emulator`, which allows the adapter to influence the environment | ||
* during dev, build and prerendering | ||
*/ | ||
emulate?: () => MaybePromise<Emulator>; | ||
emulate?: (helpers: { | ||
/** Allows to import an entry point defined within `additionalEntryPoints` by referencing its name */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** Allows to import an entry point defined within `additionalEntryPoints` by referencing its name */ | |
/** Loads an entry point defined within `additionalEntryPoints` */ |
* An object with additional entry points for Vite to consider during compilation. | ||
* The key is the name of the entry point that will be later available at `${builder.getServerDirectory()}/adapter/<name>.js`, | ||
* the value is the relative path to the entry point file. | ||
* This is useful for adapters that want to generate separate bundles for e.g. middleware. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* An object with additional entry points for Vite to consider during compilation. | |
* The key is the name of the entry point that will be later available at `${builder.getServerDirectory()}/adapter/<name>.js`, | |
* the value is the relative path to the entry point file. | |
* This is useful for adapters that want to generate separate bundles for e.g. middleware. | |
* Additional entry points that will be bundled by Vite. | |
* The key is the name of the entry point. It can be used with `importEntryPoint(name)` inside `emulate`, and the resulting module is available inside `adapt` as `${builder.getServerDirectory()}/adapter/<name>.js`. | |
* The value is the path to the entry point, relative to the current working directory. | |
* This is useful for adapters that want to generate separate bundles for e.g. middleware. |
* Runs before every request that would hit the SvelteKit runtime and before requests to static assets in dev mode. | ||
* In preview mode, in runs prior to all requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current wording is slightly confusing because the 'in dev mode' qualifier comes late. I think this rewording is a bit clearer, though it does make it sound strange that we exclude certain requests in dev
* Runs before every request that would hit the SvelteKit runtime and before requests to static assets in dev mode. | |
* In preview mode, in runs prior to all requests. | |
* During `dev`, this function runs before all requests for static assets or routes rendered by SvelteKit, but not before requests for your app's source code. | |
* During `preview`, it runs prior to all requests. |
Provides people a way to normalize a raw URL that could contain SvelteKit-internal data. One use case would be that you want to use middleware in front of, but outside of SvelteKit Extracted from #13477
Provides people a way to normalize a raw URL that could contain SvelteKit-internal data. One use case would be that you want to use middleware in front of, but outside of SvelteKit Extracted from #13477
Provides people a way to normalize a raw URL that could contain SvelteKit-internal data. One use case would be that you want to use middleware in front of, but outside of SvelteKit Extracted from #13477
|
||
You can deploy one middleware function that closely follows the [Pages Middleware API](https://developers.cloudflare.com/pages/functions/middleware/). Unlike the [handle](/docs/kit/hooks#Server-hooks-handle) hook, middleware runs on all requests, including for static assets and prerendered pages (depending on your configuration). If using [server-side route resolution](configuration#router) this means it runs prior to all navigations, no matter client- or server-side. This allows you to for example run A/B-tests on prerendered pages by rerouting a user to either variant A or B depending on a cookie. | ||
|
||
> [!NOTE] It isn't really Pages Middleware because the adapter compiles to a [single `_worker.js` file](https://developers.cloudflare.com/pages/platform/functions/#advanced-mode) (also see the [Notes](#Notes) section), which ignores middleware, but it closely mirrors its capabilities. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't really Pages Middleware because the adapter compiles to a single
_worker.js
file
Pages Middleware and Functions are all compiled to a single _worker.js
file too so I think we can omit this statement
UPDATE: protected pre-rendered routes seems viable:
Would this enable protected pre-rendered routes? #11700 For example:
I don't want to setup external Workers Routes, and instead solve everything inside the SvelteKit repo. |
This is a different take on adding middleware-like functionality to SvelteKit. In contrast to #13430 which tried to find the lowest common denominator, this PR instead makes this the responsibility of the adapters. This allows each adapter to take advantage of the unique capabilities the target platform offers, and for the APIs to be more closely aligned with the underlying platform.
So far this includes the Vercel, Netlify, Cloudflare Pages, and Node adapter:
(req, res, next) => ...
model, which allows you to for example seamlessly integrate Express or Polka middlewareThis is achieved by an enhancement to the adapter API: Adapters are now allowed to provide additional entry points to the SvelteKit build. That means for example the Vercel adapter can say "
vercel-middleware.js
at the root is another entry point", resulting in the server build output having a corresponding stable output (at.svelte-kit/output/server/vercel-middleware.js
) while at the same time being able to make use of (for example)$app/paths
within middleware. The adapter can then use the output as an entry point to bundle everything into one file and deploy it as edge middleware.For better integration with the dev environment,
beforeRequest
can be provided toemulate
. It is called before every request (in dev and preview mode) except for requests to files that are (or in case of dev would end up in)_app/immutable
.Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.