Skip to content

fix: keep default config for options passed as undefined - #97

Open
hey-research-lab wants to merge 1 commit into
Codex-Data:mainfrom
hey-research-lab:fix-undefined-config-defaults
Open

hey-research-lab wants to merge 1 commit into
Codex-Data:mainfrom
hey-research-lab:fix-undefined-config-defaults

Conversation

@hey-research-lab

Copy link
Copy Markdown

Problem

Codex.parseConfig merges the caller's config over the defaults with Object.assign({}, defaultConfig, config). Object.assign copies keys whose value is undefined, so an option that is present but undefined replaces its default. The examples in examples/simple/ construct the SDK as

const sdk = new Codex(process.env.CODEX_API_KEY || "", {
  apiUrl: process.env.CODEX_API_URL,
});

and the examples README only asks for CODEX_API_KEY. With CODEX_API_URL unset, apiUrl becomes undefined and every query and mutation fails:

const sdk = new Codex("key", { apiUrl: process.env.CODEX_API_URL, ws: false });
await sdk.send("query { getNetworks { id } }");
// TypeError: Only absolute URLs are supported   (the request goes to "undefined")

The same happens to apiRealtimeUrl, and ws: undefined turns the websocket off.

Fix

parseConfig drops options whose value is undefined before merging, so they keep their defaults. Explicit values (including ws: false) behave as before, and updateConfig goes through the same function.

Validation

  • Added a test in src/sdk/index.test.ts that passes apiUrl, apiRealtimeUrl and ws as undefined with a stub fetch: on main the request goes to "undefined" (test fails); with the fix it goes to https://graph.codex.io/graphql and the websocket client gets the default URL with ws: true.
  • pnpm test (build:types + jest): 4 suites, 93 tests pass.
  • eslint and prettier --check on the changed files: no errors.

new Codex(key, { apiUrl: process.env.CODEX_API_URL }) with the variable
unset replaced the default apiUrl with undefined, so every query and
mutation failed with "Only absolute URLs are supported". The same held
for apiRealtimeUrl and ws.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant