feat(sdk): add production HTTP controls and fix review findings - #3137
Conversation
enesgules
left a comment
There was a problem hiding this comment.
Requesting changes for one type regression. The rest looks good.
Must fix: the new overloads reject undefined options. Overloads 1 and 4 now require options, and overload 3 takes only two arguments. A caller that forwards an optional value no longer compiles (verified with tsc --strict):
const o: SearchLibraryOptions | undefined = ...;
client.searchLibrary("q", "react", o); // TS2769: No overload matches this call
client.searchLibrary("q", "react", undefined); // TS2769Both compiled before this PR. Suggested fix (inline comments below): make options optional in the JSON overload and the runtime-union overload, and drop the two-argument overload. Three overloads instead of four, and all call shapes compile. Same for getContext. Please add a type test for the Options | undefined case.
Flag: changeset is patch, but this is a breaking type change. Consumers that pass runtime-selected options and use the result as an array will fail to compile after upgrading. The new type is correct, but say so in the changeset text so users know why their build broke. At 0.x, patch is acceptable.
Verified correct:
retry: false→attempts: 0is exactly one fetch with thei <= attemptsloop. The new test proves it.- Unit/integration split is clean; fork PRs no longer need the API secret. CI is green.
requesterWithis the right size for a test double.
Nits (optional):
- The empty-input rejection is tested twice (
client.test.tsandsearch-library/index.test.ts). Keep one.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
|
Docs7 for context7.com
Commit |
enesgules
left a comment
There was a problem hiding this comment.
All seven earlier comments are addressed in 097d280 (optional options overloads, the | undefined type test, retry === false in one place, dead API_KEY fallback removed). I resolved those threads.
I also reviewed the four feature commits added after the first review (HTTP controls, redis-js alignment, transport split, retry API). Typecheck, lint, build, 36 unit tests, and the ai-sdk typecheck pass locally. The design is sound: GET-only retries, Retry-After, timeout classification during backoff, protected Authorization, and the abort-state cleanup all check out.
Must fix (one line): an error response whose body is the JSON literal null throws a raw TypeError instead of a Context7Error. Repro:
const client = new HttpClient({ baseUrl: "https://x.test", retry: false,
fetch: async () => new Response("null", { status: 500, headers: { "content-type": "application/json" } }) });
await client.request({ method: "GET", path: ["a"] });
// TypeError: Cannot read properties of null (reading 'message')Suggestion inline. A unit test next to "falls back to statusText on empty error body" would lock it in.
Non-blocking
- The changeset says timeouts are configurable but not that a 30 s default now applies to every request. Master had no timeout. One sentence in the changeset would help users who see new
request_timeouterrors. Command.requestResultnow throwsTypeErrorfor a missing result where master threwContext7Error. The path is unreachable today, butContext7Errorkeeps the public contract consistent.
enesgules
left a comment
There was a problem hiding this comment.
813bb6a addresses everything: non-object JSON error bodies are shape-checked with a regression test for the null literal, Command.requestResult throws Context7Error again, and the changeset documents the 30 s default timeout. Verified locally: typecheck, lint, build, ai-sdk typecheck, and 38 unit tests pass. CI is green.
Summary
retry: falseas exactly one request@upstash/redisSDKProduction SDK controls
@upstash/redisretry shape and default backoffRetry-Aftersupport and GET-only retries; mutating requests remain single-attemptkeepAlive, customfetch,baseUrl, additional headers, and native fetch cache settingsContext7Errorfields including status, code, request ID, rate limits, retryability, and causeAuthorizationheader sourced from the required API keyMaintainability
http/index.tsfrom 521 lines to 184 focused orchestration linesVerification
pnpm --filter @upstash/context7-sdk test(36 tests)pnpm --filter @upstash/context7-sdk test:integrationpnpm --filter @upstash/context7-sdk typecheckpnpm --filter @upstash/context7-sdk lint:checkpnpm --filter @upstash/context7-sdk buildpnpm --filter @upstash/context7-tools-ai-sdk typecheckpnpm --filter @upstash/context7-tools-ai-sdk buildAuthentication remains mandatory. Context7 intentionally extends the Redis transport convention by retrying selected transient HTTP statuses, honoring
Retry-After, and restricting automatic retries to safe requests.