chore: gate the server runtime behind configure - #17008
Conversation
|
Install the latest version of pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/7e03e39c163888ce552f9cea10e2732438450fafOpen in |
|
|
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 (2)
🔗 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; 4 remain after this review. 📝 WalkthroughWalkthroughThe server runtime now exposes Priority: ⬇️ Low — Defer this server-runtime restructuring because it changes initialization across build, development, preview, and prerender flows without supplied external urgency. Merge Risk: ⚪ Minimal · up to The configured server-instance transition preserves request handling responsibilities without identified merge-blocking risk. 🚥 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. packages/kit/src/runtime/server/instance.jsESLint skipped: the matched ESLint configuration already failed (missing-dependency). 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 |
| await configure({ building: true, manifest, env }); | ||
|
|
||
| /** @type {import('types').ServerModule} */ | ||
| const { init, respond } = await import(pathToFileURL(`${server_root}/server/index.js`).href); |
There was a problem hiding this comment.
This makes me wonder... can we invert this and make the "this stuff must happen before this other stuff" API explicit? Instead of having /server/internal.js, could we have just /server/index.js, which exports configure, and configure returns init and respond?
/** @type {import('types').ServerInternalModule} */
const { configure } = await import(pathToFileURL(`${server_root}/server/index.js`).href);
const { init, respond } = await configure({ building: true, manifest, env });Internally configure would basically just be returning import(pathToFileURL(./do-not-import-this-yourself.js).href), so what's "actually happening" is basically the same, but it would guarantee callers in these several places would have to call configure before importing the stuff that depends on what configure does.
Nic-Polumeyv
changed the title
configure
66bbaeb to
e1e2090
Compare
e1e2090 to
3dbec6d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: QUIET
Plan: Advanced
Run ID: 79aa5acf-1c2c-44b1-949a-769045503406
📒 Files selected for processing (4)
packages/kit/src/exports/vite/build/index.jspackages/kit/src/runtime/server/index.jspackages/kit/src/runtime/server/page/render.jspackages/kit/src/types/internal.d.ts
🔗 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; 6 remain after this review.
| const instance = await import('./instance.js'); | ||
| if (env) instance.set_env(env); | ||
|
|
||
| return instance; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
printf '%s\n' '--- changed file ---'
sed -n '1,110p' packages/kit/src/runtime/server/index.js
printf '%s\n' '--- configure and callers ---'
rg -n -A35 -B8 'configure|create_server|new Server|class Server' packages/kit/src packages/kit/test packages/kit/tests 2>/dev/null | head -240
printf '%s\n' '--- instance state references ---'
rg -n -A4 -B4 'set_env|manifest|read|env' packages/kit/src/runtime/server/instance.js packages/kit/src/runtime/server 2>/dev/null | head -260Repository: sveltejs/kit
Length of output: 39648
🤖 get_repo_knowledge executed:
get_repo_knowledge sveltejs/kit /tmp/coderabbit-repo-knowledge/sveltejs-kit-ab1bcef7/conventions
Length of output: 897
🏁 Script executed:
printf '%s\n' '--- module-level state definitions ---'
sed -n '1,180p' packages/kit/src/runtime/server/internal.js
printf '%s\n' '--- instance initialisation and request path ---'
sed -n '1,190p' packages/kit/src/runtime/server/instance.js
printf '%s\n' '--- configure/create_server consumers ---'
rg -n --glob '!packages/kit/src/runtime/server/index.js' '\b(configure|create_server)\b|from .*[\\/]server[\\/]index' packages/kit packages 2>/dev/null | head -180
printf '%s\n' '--- generated server instance implementation ---'
rg -n -A45 -B15 'generateServerInstance' packages/kit/src packages/kit/test 2>/dev/null | head -220Repository: sveltejs/kit
Length of output: 35337
🏁 Script executed:
printf '%s\n' '--- server API and generation documentation ---'
rg -n -i -A12 -B8 'generateServerInstance|create_server|server instance|multiple.*server|server.*multiple|ServerInitOptions' documentation packages/kit/src packages/kit/types packages/*/README.md 2>/dev/null | head -260
printf '%s\n' '--- all generated-instance call sites and adapter output ---'
rg -n -A12 -B8 'generateServerInstance\(|export const server = create_server|server\.init\(' packages/adapter-* packages/kit/src packages/kit/test 2>/dev/null | head -320Repository: sveltejs/kit
Length of output: 44154
Document or enforce the one-server-per-module-graph contract. configure() updates module-level manifest and read_implementation state, then returns cached ./instance.js. If two create_server() instances share a module graph, initialising the second can make the first use the second server's configuration. Add an interleaved two-manifest regression test or isolate state per server.
3 participants
Kit's postbuild forks, dev and preview each imported
server/internal.js, calledconfigure, and only then importedserver/index.js, because the runtime statically imports the generated env module, which evaluates the user'ssrc/envconfig, which may readbuilding. Nothing enforced that order beyond a comment in each caller, andprerender.jsset the same state three times along the way.Following #17008 (comment),
server/index.jsnow exportsconfigure, which sets the state and returns the runtime asimport('./instance.js'), so the runtime, and with it the env config, is unreachable without going through it.create_serverand theServershim stay in the entry, so adapters are untouched. With nothing able to run beforeconfigure,optionsno longer needs a setter,set_envis a static binding on the instance, and the state setting moves out of the string template inwrite_server.jsinto the entry, leaving the generated module withoptionsandget_hooks.server/internal.jsis no longer emitted.