chore: gate the server runtime behind configure - #17008
Conversation
|
Install the latest version of pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/e1e209012f2e632e8875ce2d64bc3f9d0c8b4a4cOpen in |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Advanced Run ID: 📒 Files selected for processing (20)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. Important Approval pendingCodeRabbit has no unresolved comments, but it has not reviewed the latest commit. Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.
📝 WalkthroughWalkthroughThe server runtime now exposes Sequence Diagram(s)sequenceDiagram
participant EntryPoint
participant ServerModule
participant ServerInstance
participant ResponseHandler
EntryPoint->>ServerModule: configure(server options)
ServerModule->>ServerInstance: create configured instance
EntryPoint->>ServerInstance: init()
EntryPoint->>ServerInstance: respond(request, options)
ServerInstance->>ResponseHandler: generate response
ResponseHandler-->>ServerInstance: Response
ServerInstance-->>EntryPoint: Response
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change gates server runtime initialization behind configure to enforce environment setup ordering; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
2 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.