chore: configure the server runtime in one place - #17000
Conversation
|
Install the latest version of pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/72cfe2552d8397e680a255293fd5b2e0640224a1Open in |
🦋 Changeset detectedLatest commit: 72cfe25 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: QUIET Plan: Advanced Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe server lifecycle now uses module-level Priority: ➖ Normal — Schedule the server runtime centralization because it changes initialization and request handling across build, prerender, development, preview, fallback, and public type APIs. Merge Risk: ⚪ Minimal · up to Server lifecycle setup is centralized behind module-level APIs while preserving HEAD responses without bodies. No concrete current-head merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
packages/kit/src/runtime/server/index.jsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
elliott-with-the-longest-name-on-github
left a comment
elliott-with-the-longest-name-on-github
left a comment
There was a problem hiding this comment.
Looks good, just one nit to look at before we merge
…ss with init and respond
4f69920 to
c440ed4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
…mmended `create_server()`/`respond` path, only in the deprecated `Server.respond` wrapper.
This commit fixes the issue reported at packages/kit/src/runtime/server/index.js:201
## Bug
On the `version-3` branch, HEAD-body stripping lived in the single `Server.respond` method that every adapter used:
```js
if (request.method === 'HEAD' && response.body !== null) {
response.body.cancel().catch(noop);
return new Response(null, response);
}
```
After the refactor, the shared implementation moved into the module-level `respond_to()` function, and `create_server()` returns `{ init, respond }` where `respond = respond_to` (or a serialised wrapper). Adapters generated via `builder.generateServerInstance` use this path:
- `packages/adapter-vercel/files/serverless.js` → `server.respond(request, {...})`
- `packages/adapter-cloudflare/files/worker.js` → `await server.respond(req, {...})`
- `packages/adapter-netlify/files/edge.js` → `server.respond(request, {...})`
- `packages/adapter-netlify/files/serverless.js` → `server.respond(request, {...})`
But the merge added the HEAD handling **only** to the deprecated `Server.respond` method, not to `respond_to`. So the `create_server().respond` / standalone `respond` paths did not strip HEAD bodies.
### Failure mode
A `HEAD` request routed through any edge/serverless adapter that returns the `Response` directly to the platform (no HTTP layer to discard the body). The returned `Response` includes a body, which is a regression from version-3 — wasting bandwidth and violating HTTP HEAD semantics for clients.
## Fix
Moved the HEAD-body stripping into `respond_to()` so all respond paths (`create_server().respond`, the standalone `respond` export, and the deprecated `Server.respond`) benefit. Simplified the now-redundant deprecated `Server.respond` wrapper to just delegate to `this.#server.respond`. `noop` was already imported at the top of the file.
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: teemingc <chewteeming01@gmail.com>
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
packages/kit/src/runtime/server/index.js-196-202 (1)
196-202: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winApply HEAD normalisation to the exported
respondpath.
create_serverexposes the module-levelresponddirectly. The body cancellation and bodylessResponseconversion exist only in this deprecatedServerwrapper. A directcreate_server(...).respond()call can therefore return a non-null body for aHEADrequest. Move this logic into the shared response path and add a regression test for the generated server path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: QUIET
Plan: Advanced
Run ID: fb915e0c-094b-4b84-afbe-9b967a0e4f23
📒 Files selected for processing (1)
packages/kit/src/runtime/server/index.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
sveltejs/vite-plugin-svelte(manual)vitejs/vite(manual)sveltejs/svelte(manual)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
3 participants
Kit sets up its server runtime in six places:
Server.init, prerender, analyse, dev, preview and the fallback generator. Each calls a different subset of the same module-level setters by hand, in an order four comments explain. TheServerclass itself holds no state since #16967, its constructor isset_manifest.configurein the generated server module now owns that order, andinitandrespondreplace the class. Theserverobject adapters get fromgenerateServerInstanceis unchanged and still only acceptsenvandread, so the internal optionsconfiguretakes (building,prerendering,manifest) are not reachable through it.Serverstays as a deprecated shim.