feat: accept config object in createApp() to consolidate settings (#31)#32
feat: accept config object in createApp() to consolidate settings (#31)#32Nadhila-dot merged 1 commit intomainfrom
Conversation
Allow createApp() to accept an optional config object that consolidates
server, TLS, and dev settings in one place instead of scattering them
across httpServerConfig, app.listen(), and .opt().
New usage:
const app = createApp({
server: { port: 8080, host: "0.0.0.0" },
tls: { cert: "./cert.pem", key: "./key.pem" },
dev: { hotReload: true, notify: true },
})
await app.listen()
Fully backward compatible — createApp() with no args still works,
and app.listen(options) can still override any setting per-call.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19b0dbce54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| serverConfig: { | ||
| ...(appListenDefaults.serverConfig ?? {}), | ||
| ...(listenOptions.serverConfig ?? {}), | ||
| tls: listenOptions.serverConfig?.tls ?? appListenDefaults.serverConfig?.tls, |
There was a problem hiding this comment.
Using nullish coalescing here makes it impossible to disable app-level TLS defaults for a specific listen() call. If createApp({ tls: ... }) is used and a caller passes serverConfig.tls = null (or chains .tls(null)), null is treated as missing and the default TLS config is reapplied, so the server still starts in HTTPS mode instead of HTTP. This breaks the advertised per-call override behavior.
Useful? React with 👍 / 👎.
|
|
||
| // Consolidate top-level config into a normalized shape that listen() can use as defaults. | ||
| const appConfig = { | ||
| server: config.server ?? {}, |
There was a problem hiding this comment.
createApp now dereferences config.server directly, so createApp(null) throws at startup. Before this change, extra arguments were ignored, so this is a backward-incompatible runtime break for JS callers that forward optional config values which may be null (e.g., parsed JSON/env defaults). Treating null like an omitted config would avoid this crash.
Useful? React with 👍 / 👎.
2 participants
Allow createApp() to accept an optional config object that consolidates server, TLS, and dev settings in one place instead of scattering them across httpServerConfig, app.listen(), and .opt().
New usage:
const app = createApp({
server: { port: 8080, host: "0.0.0.0" },
tls: { cert: "./cert.pem", key: "./key.pem" },
dev: { hotReload: true, notify: true },
})
await app.listen()
Fully backward compatible — createApp() with no args still works, and app.listen(options) can still override any setting per-call.