From 596d58b823adb0123d6d377730f0b5d2fba2b352 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 16 Aug 2026 22:02:06 -0700 Subject: [PATCH 1/5] Typecheck the tests and scripts, not just the shipped source `typescript/tsconfig.json` is the build project: it emits declarations under `rootDir: "src"`, and it excludes `tests`. `npm run typecheck` -- the whole of `make ts-typecheck`, and a step in both the test and release workflows -- ran only that project, so no job had ever typechecked a single test file. vitest strips types without checking them, so the test tree was unchecked from both sides. That is not a hypothetical gap. It is what let issue #737 ship: four generated methods declared a return type the runtime contradicts, and a test that reads `.meta` on one of them compiles clean here either way. It is also why no `TS1543` import-attribute diagnostic has ever surfaced. Add `tsconfig.test.json`, a typecheck-only project over src + tests + scripts, and run it from `npm run typecheck` after the build project. Two of its settings are load-bearing and easy to get wrong: - `exclude` is reset. `extends` inherits it, and an inherited "tests" entry silently drops every test file from the program even when `include` names them -- the program compiles, reports nothing, and proves nothing. - `noUncheckedIndexedAccess` is off. The shipped surface keeps it (the build project still checks src/ with it); in test code it only buys `calls[0]!` ceremony, since an undefined index fails the next assertion anyway. Turning it on surfaced 269 pre-existing errors, 117 of them after the flag above. They fall into five classes, all fixed here rather than suppressed -- no `@ts-ignore`, no `@ts-expect-error`, no blanket `as any`: - CFA narrowing to `never`: a `let captured: T | null = null` assigned only inside an MSW handler closure narrows to `null` at the assertions. Held in an object instead, so absence still fails the test. - `TS1543`: JSON fixture imports need `with { type: "json" }` under `module: NodeNext`. vite honors the attribute; the suite is green with it. - vitest `Mock` against `Pick`: give `vi.fn()` its signature type argument. - optional schema fields read without a presence assertion: assert first, so a missing field fails with a clear message. - a `Record` cast in my-notifications that erased the typing of the very fields under test. Three tests were driving paths that do not exist in the spec at all -- `/todolists/{todolistId}.json`, `/buckets/{projectId}/todolists/{todolistId}.json`, and a PUT to `/buckets/{bucketId}/todos/{todoId}.json`. openapi-fetch substitutes path strings blindly and the MSW stubs were written to match the fabricated URL, so both sides agreed and the tests proved client behavior at a URL the SDK will never emit. They now use the modelled paths (`/todolists/{id}`, `/todos/{todoId}`) with the stubs corrected to what the client actually sends; every assertion is unchanged. Three `as never` casts on `client.PUT` bodies went with them -- against a real path the bodies typecheck. `scripts/generate-services.ts` joins the program via the generator test that imports it. Its `serviceName` assignment is now a conditional expression rather than a `let` filled in from inside a loop, which is what TS could not prove definite. Output is unchanged: regenerating with this commit's generator reproduces the committed `src/generated/` tree byte for byte, and `make ts-check-drift` passes. --- typescript/README.md | 2 +- typescript/package.json | 2 +- typescript/scripts/generate-services.ts | 25 +++----- typescript/tests/auth-strategy.test.ts | 28 ++++++--- typescript/tests/client.test.ts | 51 ++++++++------- typescript/tests/hooks.test.ts | 19 ++++-- typescript/tests/integration.test.ts | 9 ++- typescript/tests/middleware-lifecycle.test.ts | 22 +++---- typescript/tests/security.test.ts | 50 ++++++++------- typescript/tests/services/boosts.test.ts | 6 +- typescript/tests/services/cards.test.ts | 2 +- .../tests/services/client-visibility.test.ts | 2 +- typescript/tests/services/comments.test.ts | 2 +- typescript/tests/services/documents.test.ts | 28 ++++++--- typescript/tests/services/gauges.test.ts | 4 +- typescript/tests/services/hill-charts.test.ts | 18 ++++-- typescript/tests/services/messages.test.ts | 2 +- .../tests/services/my-notifications.test.ts | 56 ++++++++++------- typescript/tests/services/recordings.test.ts | 25 +++++--- typescript/tests/services/schedules.test.ts | 37 +++++++---- typescript/tests/services/search.test.ts | 2 +- .../tests/services/subscriptions.test.ts | 18 ++++-- typescript/tests/services/todolists.test.ts | 6 +- typescript/tests/services/todos.test.ts | 7 ++- typescript/tests/services/tools.test.ts | 10 +-- typescript/tests/services/uploads.test.ts | 62 +++++++++++++------ typescript/tests/services/vaults.test.ts | 18 ++++-- typescript/tsconfig.test.json | 31 ++++++++++ 28 files changed, 344 insertions(+), 200 deletions(-) create mode 100644 typescript/tsconfig.test.json diff --git a/typescript/README.md b/typescript/README.md index c7e2cfce6..794c78cbd 100644 --- a/typescript/README.md +++ b/typescript/README.md @@ -923,7 +923,7 @@ npm run build # Run tests npm test -# Type check +# Type check (src via tsconfig.json, then tests and scripts via tsconfig.test.json) npm run typecheck # Lint diff --git a/typescript/package.json b/typescript/package.json index 3084fb1b2..66b343600 100644 --- a/typescript/package.json +++ b/typescript/package.json @@ -28,7 +28,7 @@ "test": "vitest run", "test:watch": "vitest", "test:coverage": "vitest run --coverage", - "typecheck": "tsc --noEmit", + "typecheck": "tsc --noEmit && tsc -p tsconfig.test.json --noEmit", "lint": "oxlint src --ignore-pattern 'src/generated/**'" }, "keywords": [ diff --git a/typescript/scripts/generate-services.ts b/typescript/scripts/generate-services.ts index e3dbfc3fc..fca9c241e 100644 --- a/typescript/scripts/generate-services.ts +++ b/typescript/scripts/generate-services.ts @@ -838,23 +838,14 @@ function groupOperations(spec: OpenAPISpec): Map { const tag = operation.tags?.[0] || "Untagged"; const parsed = parseOperation(path, method, operation); - // Determine service - let serviceName: string; - if (SERVICE_SPLITS[tag]) { - let found = false; - for (const [svc, opIds] of Object.entries(SERVICE_SPLITS[tag])) { - if (opIds.includes(operation.operationId)) { - serviceName = svc; - found = true; - break; - } - } - if (!found) { - serviceName = TAG_TO_SERVICE[tag] || tag.replace(/\s+/g, ""); - } - } else { - serviceName = TAG_TO_SERVICE[tag] || tag.replace(/\s+/g, ""); - } + // Determine service: a split tag routes the operation to the first + // sub-service that names it, and anything unlisted falls to the tag's + // own service. + const split = SERVICE_SPLITS[tag]; + const splitService = split + ? Object.entries(split).find(([, opIds]) => opIds.includes(operation.operationId))?.[0] + : undefined; + const serviceName = splitService ?? (TAG_TO_SERVICE[tag] || tag.replace(/\s+/g, "")); if (!services.has(serviceName)) { services.set(serviceName, { diff --git a/typescript/tests/auth-strategy.test.ts b/typescript/tests/auth-strategy.test.ts index 41a08657e..b91ac29d7 100644 --- a/typescript/tests/auth-strategy.test.ts +++ b/typescript/tests/auth-strategy.test.ts @@ -10,6 +10,14 @@ import { createBasecampClient } from "../src/client.js"; const BASE_URL = "https://3.basecampapi.com/12345"; +// Request capture below uses `const captured: { request?: Request } = {}` rather +// than a `let capturedRequest: Request | null = null`. The assignment happens +// inside an MSW handler closure that TypeScript's control-flow analysis cannot +// see, so the `let` form stays narrowed to `null` and every later read of it is +// typed `never`. Holding the value on an object defeats that narrowing without +// weakening anything: if the handler never runs, `captured.request` is still +// `undefined` and the header assertions still fail. + describe("BearerAuth", () => { it("sets Authorization header with static token", async () => { const auth = bearerAuth("my-token"); @@ -45,11 +53,11 @@ describe("Custom AuthStrategy", () => { }); it("works with createBasecampClient via auth option", async () => { - let capturedRequest: Request | null = null; + const captured: { request?: Request } = {}; server.use( http.get(`${BASE_URL}/projects.json`, ({ request }) => { - capturedRequest = request; + captured.request = request; return HttpResponse.json([]); }) ); @@ -67,8 +75,8 @@ describe("Custom AuthStrategy", () => { await client.GET("/projects.json"); - expect(capturedRequest?.headers.get("X-Custom-Auth")).toBe("custom-value"); - expect(capturedRequest?.headers.get("Authorization")).toBeNull(); + expect(captured.request?.headers.get("X-Custom-Auth")).toBe("custom-value"); + expect(captured.request?.headers.get("Authorization")).toBeNull(); }); }); @@ -90,11 +98,11 @@ describe("createBasecampClient auth validation", () => { }); it("accepts accessToken for backward compatibility", async () => { - let capturedRequest: Request | null = null; + const captured: { request?: Request } = {}; server.use( http.get(`${BASE_URL}/projects.json`, ({ request }) => { - capturedRequest = request; + captured.request = request; return HttpResponse.json([]); }) ); @@ -106,17 +114,17 @@ describe("createBasecampClient auth validation", () => { await client.GET("/projects.json"); - expect(capturedRequest?.headers.get("Authorization")).toBe( + expect(captured.request?.headers.get("Authorization")).toBe( "Bearer compat-token" ); }); it("accepts auth option with BearerAuth", async () => { - let capturedRequest: Request | null = null; + const captured: { request?: Request } = {}; server.use( http.get(`${BASE_URL}/projects.json`, ({ request }) => { - capturedRequest = request; + captured.request = request; return HttpResponse.json([]); }) ); @@ -128,7 +136,7 @@ describe("createBasecampClient auth validation", () => { await client.GET("/projects.json"); - expect(capturedRequest?.headers.get("Authorization")).toBe( + expect(captured.request?.headers.get("Authorization")).toBe( "Bearer auth-option-token" ); }); diff --git a/typescript/tests/client.test.ts b/typescript/tests/client.test.ts index b3564697a..307d6b898 100644 --- a/typescript/tests/client.test.ts +++ b/typescript/tests/client.test.ts @@ -12,6 +12,14 @@ import { DEFAULT_MAX_PAGES } from "../src/pagination-utils.js"; const BASE_URL = "https://3.basecampapi.com/12345"; +// Request capture below uses `const captured: { request?: Request } = {}` rather +// than a `let capturedRequest: Request | null = null`. The assignment happens +// inside an MSW handler closure that TypeScript's control-flow analysis cannot +// see, so the `let` form stays narrowed to `null` and every later read of it is +// typed `never`. Holding the value on an object defeats that narrowing without +// weakening anything: if the handler never runs, `captured.request` is still +// `undefined` and the header assertions still fail. + describe("BasecampClient", () => { beforeEach(() => { vi.clearAllMocks(); @@ -19,11 +27,11 @@ describe("BasecampClient", () => { describe("authentication", () => { it("should add Authorization header to requests", async () => { - let capturedRequest: Request | null = null; + const captured: { request?: Request } = {}; server.use( http.get(`${BASE_URL}/projects.json`, ({ request }) => { - capturedRequest = request; + captured.request = request; return HttpResponse.json([]); }) ); @@ -35,17 +43,17 @@ describe("BasecampClient", () => { await client.GET("/projects.json"); - expect(capturedRequest?.headers.get("Authorization")).toBe( + expect(captured.request?.headers.get("Authorization")).toBe( "Bearer test-token" ); }); it("should support async token provider", async () => { - let capturedRequest: Request | null = null; + const captured: { request?: Request } = {}; server.use( http.get(`${BASE_URL}/projects.json`, ({ request }) => { - capturedRequest = request; + captured.request = request; return HttpResponse.json([]); }) ); @@ -60,7 +68,7 @@ describe("BasecampClient", () => { await client.GET("/projects.json"); expect(tokenProvider).toHaveBeenCalled(); - expect(capturedRequest?.headers.get("Authorization")).toBe( + expect(captured.request?.headers.get("Authorization")).toBe( "Bearer dynamic-token" ); }); @@ -70,11 +78,11 @@ describe("BasecampClient", () => { it("should not set Content-Type on bodyless GET requests", async () => { // bc3 silently discards query params on GET requests that carry a // Content-Type header, so bodyless requests must not send one. - let capturedRequest: Request | null = null; + const captured: { request?: Request } = {}; server.use( http.get(`${BASE_URL}/projects.json`, ({ request }) => { - capturedRequest = request; + captured.request = request; return HttpResponse.json([]); }) ); @@ -86,16 +94,16 @@ describe("BasecampClient", () => { await client.GET("/projects.json"); - expect(capturedRequest?.headers.get("Content-Type")).toBeNull(); - expect(capturedRequest?.headers.get("Accept")).toBe("application/json"); + expect(captured.request?.headers.get("Content-Type")).toBeNull(); + expect(captured.request?.headers.get("Accept")).toBe("application/json"); }); it("should set Content-Type to application/json for JSON bodies", async () => { - let capturedRequest: Request | null = null; + const captured: { request?: Request } = {}; server.use( http.post(`${BASE_URL}/todolists/456/todos.json`, ({ request }) => { - capturedRequest = request; + captured.request = request; return HttpResponse.json({ id: 1, content: "Test todo" }, { status: 201 }); }) ); @@ -110,15 +118,15 @@ describe("BasecampClient", () => { body: { content: "Test todo" }, }); - expect(capturedRequest?.headers.get("Content-Type")).toBe("application/json"); + expect(captured.request?.headers.get("Content-Type")).toBe("application/json"); }); it("should preserve an explicitly set Content-Type on requests with a body", async () => { - let capturedRequest: Request | null = null; + const captured: { request?: Request } = {}; server.use( http.post(`${BASE_URL}/todolists/456/todos.json`, ({ request }) => { - capturedRequest = request; + captured.request = request; return HttpResponse.json({ id: 1, content: "Test todo" }, { status: 201 }); }) ); @@ -134,7 +142,7 @@ describe("BasecampClient", () => { headers: { "Content-Type": "application/json; charset=utf-8" }, }); - expect(capturedRequest?.headers.get("Content-Type")).toBe( + expect(captured.request?.headers.get("Content-Type")).toBe( "application/json; charset=utf-8" ); }); @@ -469,7 +477,7 @@ describe("BasecampClient", () => { it("should return error for 404", async () => { server.use( - http.get(`${BASE_URL}/todolists/999.json`, () => { + http.get(`${BASE_URL}/todolists/999`, () => { return HttpResponse.json( { error: "Not found" }, { status: 404 } @@ -482,12 +490,9 @@ describe("BasecampClient", () => { accessToken: "test-token", }); - const { data, error } = await client.GET( - "/todolists/{todolistId}.json", - { - params: { path: { todolistId: 999 } }, - } - ); + const { data, error } = await client.GET("/todolists/{id}", { + params: { path: { id: 999 } }, + }); expect(data).toBeUndefined(); expect(error).toBeDefined(); diff --git a/typescript/tests/hooks.test.ts b/typescript/tests/hooks.test.ts index d367a26e4..657ebc926 100644 --- a/typescript/tests/hooks.test.ts +++ b/typescript/tests/hooks.test.ts @@ -1,7 +1,7 @@ /** * Tests for the hooks module */ -import { describe, it, expect, vi, beforeEach } from "vitest"; +import { describe, it, expect, vi, beforeEach, type Mock } from "vitest"; import { chainHooks, consoleHooks, @@ -176,13 +176,22 @@ describe("chainHooks", () => { }); describe("consoleHooks", () => { - let mockLogger: { log: ReturnType; warn: ReturnType; error: ReturnType }; + // consoleHooks takes `Pick`. A bare vi.fn() + // infers the catch-all `Mock`, which is not + // assignable to a console method, so each mock is given the signature of the + // method it stands in for — the substitute still has to satisfy the real + // contract. + let mockLogger: { + log: Mock; + warn: Mock; + error: Mock; + }; beforeEach(() => { mockLogger = { - log: vi.fn(), - warn: vi.fn(), - error: vi.fn(), + log: vi.fn(), + warn: vi.fn(), + error: vi.fn(), }; }); diff --git a/typescript/tests/integration.test.ts b/typescript/tests/integration.test.ts index 15e3948c2..626b1e671 100644 --- a/typescript/tests/integration.test.ts +++ b/typescript/tests/integration.test.ts @@ -164,7 +164,7 @@ describe("Integration", () => { let callCount = 0; server.use( - http.get(`${BASE_URL}/buckets/123/todolists/999.json`, () => { + http.get(`${BASE_URL}/todolists/999`, () => { callCount++; return HttpResponse.json( { error: "Not found" }, @@ -187,10 +187,9 @@ describe("Integration", () => { enableCache: true, }); - const { error } = await client.GET( - "/buckets/{projectId}/todolists/{todolistId}.json", - { params: { path: { projectId: 123, todolistId: 999 } } } - ); + const { error } = await client.GET("/todolists/{id}", { + params: { path: { id: 999 } }, + }); expect(error).toBeDefined(); diff --git a/typescript/tests/middleware-lifecycle.test.ts b/typescript/tests/middleware-lifecycle.test.ts index 02d35dde5..cb395b985 100644 --- a/typescript/tests/middleware-lifecycle.test.ts +++ b/typescript/tests/middleware-lifecycle.test.ts @@ -89,7 +89,7 @@ describe("middleware request lifecycle", () => { let arrived = 0; server.use( - http.put(`${BASE_URL}/buckets/1/todos/2.json`, async ({ request }) => { + http.put(`${BASE_URL}/todos/2`, async ({ request }) => { const body = await request.text(); const n = ++arrived; @@ -119,14 +119,14 @@ describe("middleware request lifecycle", () => { }); await Promise.all([ - client.PUT("/buckets/{bucketId}/todos/{todoId}.json", { - params: { path: { bucketId: 1, todoId: 2 } }, + client.PUT("/todos/{todoId}", { + params: { path: { todoId: 2 } }, body: { content: "AAA" }, - } as never), - client.PUT("/buckets/{bucketId}/todos/{todoId}.json", { - params: { path: { bucketId: 1, todoId: 2 } }, + }), + client.PUT("/todos/{todoId}", { + params: { path: { todoId: 2 } }, body: { content: "BBB" }, - } as never), + }), ]); const firstBodies = seen.filter((s) => s.phase === "initial").map((s) => s.body).sort(); @@ -656,7 +656,7 @@ describe("middleware request lifecycle", () => { const bodies: string[] = []; let attempts = 0; server.use( - http.put(`${BASE_URL}/buckets/1/todos/2.json`, async ({ request }) => { + http.put(`${BASE_URL}/todos/2`, async ({ request }) => { attempts++; bodies.push(await request.text()); if (attempts <= 2) { @@ -674,10 +674,10 @@ describe("middleware request lifecycle", () => { accessToken: "test-token", }); - await client.PUT("/buckets/{bucketId}/todos/{todoId}.json", { - params: { path: { bucketId: 1, todoId: 2 } }, + await client.PUT("/todos/{todoId}", { + params: { path: { todoId: 2 } }, body: { content: "same-bytes" }, - } as never); + }); expect(attempts).toBe(3); expect(bodies).toHaveLength(3); diff --git a/typescript/tests/security.test.ts b/typescript/tests/security.test.ts index 329ae5806..0f8039463 100644 --- a/typescript/tests/security.test.ts +++ b/typescript/tests/security.test.ts @@ -27,6 +27,14 @@ import { discover } from "../src/oauth/discovery.js"; const BASE_URL = "https://3.basecampapi.com/12345"; +// fetchAllPages/paginateAll type their page parser as `(r) => Promise`, but +// `Response#json()` is typed `Promise` — it cannot know the body is an +// array. Narrow once here instead of at each of the ~20 call sites below; these +// tests are about Link-header handling and page caps, not about body shape, and +// every stubbed body in this file really is a JSON array. +const parsePages = (r: Response): Promise => + r.json() as Promise; + // ============================================================================= // Link Header Origin Validation (SSRF / Token Leakage) // ============================================================================= @@ -46,7 +54,7 @@ describe("Link header origin validation", () => { }); await expect( - fetchAllPages(response, (r) => r.json()) + fetchAllPages(response, parsePages) ).rejects.toThrow("different origin"); }); @@ -61,7 +69,7 @@ describe("Link header origin validation", () => { value: "https://3.basecampapi.com/12345/projects.json", }); - const generator = paginateAll(response, (r) => r.json()); + const generator = paginateAll(response, parsePages); // First yield should succeed (initial page) const first = await generator.next(); @@ -83,7 +91,7 @@ describe("Link header origin validation", () => { value: "https://3.basecampapi.com/12345/projects.json", }); - const results = await fetchAllPages(response, (r) => r.json()); + const results = await fetchAllPages(response, parsePages); expect(results).toEqual([{ id: 1 }]); }); @@ -114,7 +122,7 @@ describe("Link header origin validation", () => { value: "https://3.basecampapi.com/12345/projects.json", }); - const results = await fetchAllPages(response, (r) => r.json()); + const results = await fetchAllPages(response, parsePages); // Should have fetched page 2 (relative URL resolved to same origin) expect(results).toEqual([{ id: 1 }, { id: 2 }]); expect(fetchCallCount).toBe(1); @@ -148,7 +156,7 @@ describe("Link header origin validation", () => { value: "https://3.basecampapi.com/12345/projects.json", }); - const results = await fetchAllPages(response, (r) => r.json()); + const results = await fetchAllPages(response, parsePages); expect(results).toEqual([{ id: 1 }, { id: 2 }]); expect(fetchCallCount).toBe(1); } finally { @@ -195,7 +203,7 @@ describe("Link header origin validation", () => { value: "https://3.basecampapi.com/v1/projects", }); - const results = await fetchAllPages(response, (r) => r.json()); + const results = await fetchAllPages(response, parsePages); expect(results).toEqual([{ id: 1 }, { id: 2 }, { id: 3 }]); // Page 2 URL resolved from initial expect(fetchedUrls[0]).toBe("https://3.basecampapi.com/v1/projects?page=2"); @@ -275,7 +283,7 @@ describe("pagination page cap", () => { it("makes no further request at all when maxPages is 1", async () => { const mock = installEndlessFetch(); try { - const results = await fetchAllPages(firstOfEndless(), (r) => r.json(), undefined, 1); + const results = await fetchAllPages(firstOfEndless(), parsePages, undefined, 1); expect(results).toEqual([{ id: 1 }]); // The initial response was supplied by the caller. One page consumed @@ -290,7 +298,7 @@ describe("pagination page cap", () => { it("consumes exactly maxPages pages against a server that never stops", async () => { const mock = installEndlessFetch(); try { - const results = await fetchAllPages(firstOfEndless(), (r) => r.json(), undefined, 3); + const results = await fetchAllPages(firstOfEndless(), parsePages, undefined, 3); expect(results).toEqual([{ id: 1 }, { id: 2 }, { id: 3 }]); expect(mock.count()).toBe(2); @@ -315,7 +323,7 @@ describe("pagination page cap", () => { try { const results = await fetchAllPages( endlessPage(selfUrl, 1, selfUrl), - (r) => r.json(), + parsePages, undefined, 3 ); @@ -337,7 +345,7 @@ describe("pagination page cap", () => { try { // Generous cap, two-page sequence: the cap must not be what ends it. - const results = await fetchAllPages(firstOfEndless(), (r) => r.json(), undefined, 100); + const results = await fetchAllPages(firstOfEndless(), parsePages, undefined, 100); expect(results).toEqual([{ id: 1 }, { id: 2 }]); expect(fetchCallCount).toBe(1); @@ -355,7 +363,7 @@ describe("pagination page cap", () => { }); try { - const results = await fetchAllPages(firstOfEndless(), (r) => r.json()); + const results = await fetchAllPages(firstOfEndless(), parsePages); expect(results).toEqual([{ id: 1 }, { id: 2 }]); expect(fetchCallCount).toBe(1); @@ -377,7 +385,7 @@ describe("pagination page cap", () => { it("makes no further request at all when maxPages is 1", async () => { const mock = installEndlessFetch(); try { - const pages = await collect(paginateAll(firstOfEndless(), (r) => r.json(), undefined, 1)); + const pages = await collect(paginateAll(firstOfEndless(), parsePages, undefined, 1)); expect(pages).toEqual([[{ id: 1 }]]); expect(mock.count()).toBe(0); @@ -389,7 +397,7 @@ describe("pagination page cap", () => { it("yields exactly maxPages pages against a server that never stops", async () => { const mock = installEndlessFetch(); try { - const pages = await collect(paginateAll(firstOfEndless(), (r) => r.json(), undefined, 3)); + const pages = await collect(paginateAll(firstOfEndless(), parsePages, undefined, 3)); expect(pages).toEqual([[{ id: 1 }], [{ id: 2 }], [{ id: 3 }]]); expect(mock.count()).toBe(2); @@ -411,7 +419,7 @@ describe("pagination page cap", () => { try { const pages = await collect( - paginateAll(endlessPage(selfUrl, 1, selfUrl), (r) => r.json(), undefined, 3) + paginateAll(endlessPage(selfUrl, 1, selfUrl), parsePages, undefined, 3) ); expect(pages).toEqual([[{ id: 1 }], [{ id: 1 }], [{ id: 1 }]]); @@ -430,7 +438,7 @@ describe("pagination page cap", () => { }); try { - const pages = await collect(paginateAll(firstOfEndless(), (r) => r.json(), undefined, 100)); + const pages = await collect(paginateAll(firstOfEndless(), parsePages, undefined, 100)); expect(pages).toEqual([[{ id: 1 }], [{ id: 2 }]]); expect(fetchCallCount).toBe(1); @@ -448,7 +456,7 @@ describe("pagination page cap", () => { }); try { - const pages = await collect(paginateAll(firstOfEndless(), (r) => r.json())); + const pages = await collect(paginateAll(firstOfEndless(), parsePages)); expect(pages).toEqual([[{ id: 1 }], [{ id: 2 }]]); expect(fetchCallCount).toBe(1); @@ -532,7 +540,7 @@ describe("pagination page cap", () => { it.each(INVALID)("rejects %s with a usage error and fetches nothing", async (_label, value) => { const mock = installCountingFetch(); try { - const error = await fetchAllPages(firstOfEndless(), (r) => r.json(), undefined, value) + const error = await fetchAllPages(firstOfEndless(), parsePages, undefined, value) .then(() => null) .catch((e: unknown) => e); @@ -563,13 +571,13 @@ describe("pagination page cap", () => { it.each(INVALID)("rejects %s eagerly and fetches nothing", (_label, value) => { const mock = installCountingFetch(); try { - expect(() => paginateAll(firstOfEndless(), (r) => r.json(), undefined, value)).toThrow( + expect(() => paginateAll(firstOfEndless(), parsePages, undefined, value)).toThrow( BasecampError ); let thrown: unknown; try { - paginateAll(firstOfEndless(), (r) => r.json(), undefined, value); + paginateAll(firstOfEndless(), parsePages, undefined, value); } catch (e: unknown) { thrown = e; } @@ -590,8 +598,8 @@ describe("pagination page cap", () => { it.each([1, 2, 3, 100, 10_000])("accepts the valid cap %i", async (value) => { const mock = installCountingFetch(); try { - const results = await fetchAllPages(firstOfEndless(), (r) => r.json(), undefined, value); - const pages = await collect(paginateAll(firstOfEndless(), (r) => r.json(), undefined, value)); + const results = await fetchAllPages(firstOfEndless(), parsePages, undefined, value); + const pages = await collect(paginateAll(firstOfEndless(), parsePages, undefined, value)); // A two-page sequence: page 1 is supplied, page 2 is terminal. const expected = value === 1 ? [{ id: 1 }] : [{ id: 1 }, { id: 2 }]; diff --git a/typescript/tests/services/boosts.test.ts b/typescript/tests/services/boosts.test.ts index db72295df..446ab09f9 100644 --- a/typescript/tests/services/boosts.test.ts +++ b/typescript/tests/services/boosts.test.ts @@ -42,7 +42,11 @@ describe("BoostsService", () => { const boost = await client.boosts.get(boostId); expect(boost.id).toBe(boostId); expect(boost.content).toBe("🎉"); - expect(boost.booster.name).toBe("Jane Doe"); + // `booster` is optional in the generated schema, so assert it arrived + // before reading through it: a boost served without one then fails on + // the presence check rather than on an undefined property read. + expect(boost.booster).toBeDefined(); + expect(boost.booster?.name).toBe("Jane Doe"); }); it("should throw not_found for missing boost", async () => { diff --git a/typescript/tests/services/cards.test.ts b/typescript/tests/services/cards.test.ts index 05c14f8a6..84eaa57c5 100644 --- a/typescript/tests/services/cards.test.ts +++ b/typescript/tests/services/cards.test.ts @@ -7,7 +7,7 @@ import { server } from "../setup.js"; import { createBasecampClient } from "../../src/client.js"; import { BasecampError } from "../../src/errors.js"; import type { BasecampClient } from "../../src/client.js"; -import cardFixture from "../../../spec/fixtures/cards/get.json"; +import cardFixture from "../../../spec/fixtures/cards/get.json" with { type: "json" }; const BASE_URL = "https://3.basecampapi.com/12345"; diff --git a/typescript/tests/services/client-visibility.test.ts b/typescript/tests/services/client-visibility.test.ts index 750ab19b1..4f4e3b87d 100644 --- a/typescript/tests/services/client-visibility.test.ts +++ b/typescript/tests/services/client-visibility.test.ts @@ -6,7 +6,7 @@ import { http, HttpResponse } from "msw"; import { server } from "../setup.js"; import { createBasecampClient } from "../../src/client.js"; import type { BasecampClient } from "../../src/client.js"; -import recordingFixture from "../../../spec/fixtures/recordings/get.json"; +import recordingFixture from "../../../spec/fixtures/recordings/get.json" with { type: "json" }; const BASE_URL = "https://3.basecampapi.com/12345"; diff --git a/typescript/tests/services/comments.test.ts b/typescript/tests/services/comments.test.ts index 6b7c13672..f376fb7c7 100644 --- a/typescript/tests/services/comments.test.ts +++ b/typescript/tests/services/comments.test.ts @@ -7,7 +7,7 @@ import { server } from "../setup.js"; import { createBasecampClient } from "../../src/client.js"; import { BasecampError } from "../../src/errors.js"; import type { BasecampClient } from "../../src/client.js"; -import commentFixture from "../../../spec/fixtures/comments/get.json"; +import commentFixture from "../../../spec/fixtures/comments/get.json" with { type: "json" }; const BASE_URL = "https://3.basecampapi.com/12345"; diff --git a/typescript/tests/services/documents.test.ts b/typescript/tests/services/documents.test.ts index 87dcce28e..8f20f6960 100644 --- a/typescript/tests/services/documents.test.ts +++ b/typescript/tests/services/documents.test.ts @@ -19,6 +19,7 @@ */ import { describe, it, expect, vi, beforeEach } from "vitest"; import { http, HttpResponse } from "msw"; +import type { JsonBodyType } from "msw"; import { server } from "../setup.js"; import type { DocumentsService } from "../../src/services/documents-extensions.js"; import { BasecampError } from "../../src/errors.js"; @@ -169,17 +170,20 @@ describe("DocumentsService", () => { }); it("should send all fields in request body", async () => { - let capturedBody: { - title?: string; - content?: string; - status?: string; - } | null = null; + // Held in an object rather than a bare `let`: TS's control-flow analysis + // cannot see the assignment inside the MSW handler closure, so a + // `let x: T | null = null` narrows to `null` at the assertions below and + // every property read becomes an error on `never`. A property of a const + // object is not narrowed that way. + const captured: { + body?: { title?: string; content?: string; status?: string }; + } = {}; server.use( http.post( `${BASE_URL}/vaults/1001/documents.json`, async ({ request }) => { - capturedBody = (await request.json()) as { + captured.body = (await request.json()) as { title?: string; content?: string; status?: string; @@ -195,9 +199,9 @@ describe("DocumentsService", () => { status: "drafted", }); - expect(capturedBody?.title).toBe("Test Doc"); - expect(capturedBody?.content).toBe("

Hello

"); - expect(capturedBody?.status).toBe("drafted"); + expect(captured.body?.title).toBe("Test Doc"); + expect(captured.body?.content).toBe("

Hello

"); + expect(captured.body?.status).toBe("drafted"); }); // Client-side validation short-circuits before any HTTP call. No MSW handler @@ -560,7 +564,11 @@ describe("DocumentsService", () => { const writableStrings = ["title", "content"] as const; // Serve a GET carrying `body` and a PUT that records that it happened. - const serve = (body: unknown, requests: string[]) => { + // `body` is typed as MSW's own response-body type rather than `unknown`: + // the malformed-*envelope* cases below deliberately serve arrays, scalars + // and null, so the parameter has to stay as wide as JSON itself -- but no + // wider, or the `HttpResponse.json` call cannot accept it. + const serve = (body: JsonBodyType, requests: string[]) => { server.use( http.get(`${BASE_URL}/documents/5001`, () => { requests.push("GET"); diff --git a/typescript/tests/services/gauges.test.ts b/typescript/tests/services/gauges.test.ts index 85925eec8..72040e029 100644 --- a/typescript/tests/services/gauges.test.ts +++ b/typescript/tests/services/gauges.test.ts @@ -23,8 +23,8 @@ import type { CreateGaugeNeedleGaugeRequest, ToggleGaugeGaugeRequest, } from "../../src/generated/services/gauges.js"; -import gaugeFixture from "../../../spec/fixtures/gauges/get.json"; -import needleFixture from "../../../spec/fixtures/gauges/needle_get.json"; +import gaugeFixture from "../../../spec/fixtures/gauges/get.json" with { type: "json" }; +import needleFixture from "../../../spec/fixtures/gauges/needle_get.json" with { type: "json" }; const BASE_URL = "https://3.basecampapi.com/12345"; diff --git a/typescript/tests/services/hill-charts.test.ts b/typescript/tests/services/hill-charts.test.ts index df24bb9a6..2e18d5e7e 100644 --- a/typescript/tests/services/hill-charts.test.ts +++ b/typescript/tests/services/hill-charts.test.ts @@ -77,8 +77,13 @@ describe("HillChartsService", () => { const result = await client.hillCharts.get(todosetId); expect(result.enabled).toBe(true); expect(result.stale).toBe(false); - expect(result.dots).toHaveLength(1); - expect(result.dots[0].label).toBe("Background and research"); + // `dots` is optional in the generated schema. Assert presence first so a + // response that omits it fails here, then read through the narrowed + // local rather than repeating `?.` on every dot. + expect(result.dots).toBeDefined(); + const dots = result.dots!; + expect(dots).toHaveLength(1); + expect(dots[0].label).toBe("Background and research"); expect(result.app_versions_url).toBe(`https://3.basecamp.com/12345/buckets/100/todosets/42/hill/versions`); }); @@ -111,9 +116,12 @@ describe("HillChartsService", () => { }); expect(capturedBody).toEqual({ tracked: [1069479573], untracked: [1069479511] }); expect(result.enabled).toBe(true); - expect(result.dots).toHaveLength(2); - expect(result.dots[1].label).toBe("Design mockups"); - expect(result.dots[1].position).toBe(42); + // See the note in `get`: `dots` is optional, so assert presence first. + expect(result.dots).toBeDefined(); + const dots = result.dots!; + expect(dots).toHaveLength(2); + expect(dots[1].label).toBe("Design mockups"); + expect(dots[1].position).toBe(42); }); it("should throw not_found for missing todoset", async () => { diff --git a/typescript/tests/services/messages.test.ts b/typescript/tests/services/messages.test.ts index f007b761b..34e1ae512 100644 --- a/typescript/tests/services/messages.test.ts +++ b/typescript/tests/services/messages.test.ts @@ -7,7 +7,7 @@ import { server } from "../setup.js"; import { createBasecampClient } from "../../src/client.js"; import { BasecampError } from "../../src/errors.js"; import type { BasecampClient } from "../../src/client.js"; -import messageFixture from "../../../spec/fixtures/messages/get.json"; +import messageFixture from "../../../spec/fixtures/messages/get.json" with { type: "json" }; const BASE_URL = "https://3.basecampapi.com/12345"; diff --git a/typescript/tests/services/my-notifications.test.ts b/typescript/tests/services/my-notifications.test.ts index f393f3e63..ccee7c10a 100644 --- a/typescript/tests/services/my-notifications.test.ts +++ b/typescript/tests/services/my-notifications.test.ts @@ -48,13 +48,21 @@ describe("MyNotificationsService", () => { ); const result = await client.myNotifications.myNotifications(); - const creator = (result as Record).unreads[0] as Record; - const creatorObj = creator.creator as Record; - - expect(creatorObj.id).toBe(0); - expect(typeof creatorObj.id).toBe("number"); - expect(creatorObj.system_label).toBe("basecamp"); - expect(creatorObj.personable_type).toBe("LocalPerson"); + // Assert on the response's real members rather than casting it to + // `Record`. The response is a fixed-shape struct and + // the test never iterates arbitrary keys -- the cast existed only to + // index `unreads` by string, and it threw away the typing of exactly the + // fields under test. `unreads` and `creator` are both optional in the + // schema, so assert presence first: a response missing either now fails + // on the presence check instead of on an undefined property read. + expect(result.unreads).toBeDefined(); + const creator = result.unreads![0].creator; + expect(creator).toBeDefined(); + + expect(creator!.id).toBe(0); + expect(typeof creator!.id).toBe("number"); + expect(creator!.system_label).toBe("basecamp"); + expect(creator!.personable_type).toBe("LocalPerson"); }); it("should leave numeric string creator.id as number", async () => { @@ -83,12 +91,14 @@ describe("MyNotificationsService", () => { ); const result = await client.myNotifications.myNotifications(); - const creator = (result as Record).unreads[0] as Record; - const creatorObj = creator.creator as Record; - - expect(creatorObj.id).toBe(99999); - expect(typeof creatorObj.id).toBe("number"); - expect(creatorObj.system_label).toBeUndefined(); + // See the note in the first case for why this reads the typed members. + expect(result.unreads).toBeDefined(); + const creator = result.unreads![0].creator; + expect(creator).toBeDefined(); + + expect(creator!.id).toBe(99999); + expect(typeof creator!.id).toBe("number"); + expect(creator!.system_label).toBeUndefined(); }); it("should treat junk string as sentinel", async () => { @@ -117,12 +127,14 @@ describe("MyNotificationsService", () => { ); const result = await client.myNotifications.myNotifications(); - const creator = (result as Record).unreads[0] as Record; - const creatorObj = creator.creator as Record; + // See the note in the first case for why this reads the typed members. + expect(result.unreads).toBeDefined(); + const creator = result.unreads![0].creator; + expect(creator).toBeDefined(); // "123abc" is not a valid ID — treated as sentinel - expect(creatorObj.id).toBe(0); - expect(creatorObj.system_label).toBe("123abc"); + expect(creator!.id).toBe(0); + expect(creator!.system_label).toBe("123abc"); }); it("should treat overflow numeric string as sentinel (JS cannot represent losslessly)", async () => { @@ -151,12 +163,14 @@ describe("MyNotificationsService", () => { ); const result = await client.myNotifications.myNotifications(); - const creator = (result as Record).unreads[0] as Record; - const creatorObj = creator.creator as Record; + // See the note in the first case for why this reads the typed members. + expect(result.unreads).toBeDefined(); + const creator = result.unreads![0].creator; + expect(creator).toBeDefined(); // Overflow can't be represented as a safe integer — preserved as label - expect(creatorObj.id).toBe(0); - expect(creatorObj.system_label).toBe("9223372036854775808"); + expect(creator!.id).toBe(0); + expect(creator!.system_label).toBe("9223372036854775808"); }); }); diff --git a/typescript/tests/services/recordings.test.ts b/typescript/tests/services/recordings.test.ts index 7d29b8d59..cac1d3f3a 100644 --- a/typescript/tests/services/recordings.test.ts +++ b/typescript/tests/services/recordings.test.ts @@ -63,11 +63,15 @@ describe("RecordingsService", () => { }); it("should include optional filters in query", async () => { - let capturedUrl: URL | null = null; + // Held in an object, not a `let`: control-flow analysis cannot see the + // assignment inside the handler closure, so a `let ... = null` binding + // narrows to `null`, so reading `.searchParams` off it is a `never`. The + // optional chaining still makes an unrun handler fail the assertions. + const captured: { url?: URL } = {}; server.use( http.get(`${BASE_URL}/projects/recordings.json`, ({ request }) => { - capturedUrl = new URL(request.url); + captured.url = new URL(request.url); return HttpResponse.json([]); }), ); @@ -80,26 +84,27 @@ describe("RecordingsService", () => { direction: "asc", }); - expect(capturedUrl?.searchParams.get("type")).toBe("Document"); - expect(capturedUrl?.searchParams.get("bucket")).toBe("123"); - expect(capturedUrl?.searchParams.get("status")).toBe("archived"); - expect(capturedUrl?.searchParams.get("sort")).toBe("updated_at"); - expect(capturedUrl?.searchParams.get("direction")).toBe("asc"); + expect(captured.url?.searchParams.get("type")).toBe("Document"); + expect(captured.url?.searchParams.get("bucket")).toBe("123"); + expect(captured.url?.searchParams.get("status")).toBe("archived"); + expect(captured.url?.searchParams.get("sort")).toBe("updated_at"); + expect(captured.url?.searchParams.get("direction")).toBe("asc"); }); it("should join multiple bucket IDs as CSV", async () => { - let capturedUrl: URL | null = null; + // Object-held for the same reason as above. + const captured: { url?: URL } = {}; server.use( http.get(`${BASE_URL}/projects/recordings.json`, ({ request }) => { - capturedUrl = new URL(request.url); + captured.url = new URL(request.url); return HttpResponse.json([]); }), ); await service.list("Todo", { bucket: [1, 2, 3] }); - expect(capturedUrl?.searchParams.get("bucket")).toBe("1,2,3"); + expect(captured.url?.searchParams.get("bucket")).toBe("1,2,3"); }); it("should return empty ListResult when no recordings", async () => { diff --git a/typescript/tests/services/schedules.test.ts b/typescript/tests/services/schedules.test.ts index 365a82f9c..f43aab615 100644 --- a/typescript/tests/services/schedules.test.ts +++ b/typescript/tests/services/schedules.test.ts @@ -24,6 +24,7 @@ */ import { describe, it, expect, vi, beforeEach } from "vitest"; import { http, HttpResponse } from "msw"; +import type { JsonBodyType } from "msw"; import { server } from "../setup.js"; import type { SchedulesService } from "../../src/services/schedules-extensions.js"; import { BasecampError } from "../../src/errors.js"; @@ -224,13 +225,17 @@ describe("SchedulesService", () => { }); it("should send all fields in request body", async () => { - let capturedBody: Record | null = null; + // Held in an object, not a `let`: control-flow analysis cannot see the + // assignment inside the handler closure, so a `let ... = null` binding + // narrows to `null` and every field read below becomes `never`. The + // optional chaining still makes an unrun handler fail the assertions. + const captured: { body?: Record } = {}; server.use( http.post( `${BASE_URL}/schedules/4001/entries.json`, async ({ request }) => { - capturedBody = (await request.json()) as Record; + captured.body = (await request.json()) as Record; return HttpResponse.json({ id: 1, summary: "Test" }); }, ), @@ -246,13 +251,13 @@ describe("SchedulesService", () => { notify: true, }); - expect(capturedBody?.summary).toBe("Test Event"); - expect(capturedBody?.starts_at).toBe("2024-12-20T14:00:00Z"); - expect(capturedBody?.ends_at).toBe("2024-12-20T15:00:00Z"); - expect(capturedBody?.description).toBe("

Description

"); - expect(capturedBody?.participant_ids).toEqual([1001, 1002]); - expect(capturedBody?.all_day).toBe(true); - expect(capturedBody?.notify).toBe(true); + expect(captured.body?.summary).toBe("Test Event"); + expect(captured.body?.starts_at).toBe("2024-12-20T14:00:00Z"); + expect(captured.body?.ends_at).toBe("2024-12-20T15:00:00Z"); + expect(captured.body?.description).toBe("

Description

"); + expect(captured.body?.participant_ids).toEqual([1001, 1002]); + expect(captured.body?.all_day).toBe(true); + expect(captured.body?.notify).toBe(true); }); // Client-side validation short-circuits before any HTTP call. No MSW handler @@ -864,11 +869,13 @@ describe("SchedulesService", () => { }); it("should send include_due_assignments in request body", async () => { - let capturedBody: { include_due_assignments?: boolean } | null = null; + // Object-held for the same reason as above: a `let ... = null` captured + // only inside the handler closure narrows to `null`. + const captured: { body?: { include_due_assignments?: boolean } } = {}; server.use( http.put(`${BASE_URL}/schedules/4001`, async ({ request }) => { - capturedBody = (await request.json()) as { + captured.body = (await request.json()) as { include_due_assignments?: boolean; }; return HttpResponse.json({ id: 4001, title: "Schedule" }); @@ -877,7 +884,7 @@ describe("SchedulesService", () => { await service.updateSettings(4001, { includeDueAssignments: true }); - expect(capturedBody?.include_due_assignments).toBe(true); + expect(captured.body?.include_due_assignments).toBe(true); }); }); @@ -911,7 +918,11 @@ describe("SchedulesService", () => { const writableStrings = ["summary", "starts_at", "ends_at", "description"] as const; // Serve a GET carrying `body` and a PUT that records that it happened. - const serve = (body: unknown, requests: string[]) => { + // `body` is whatever a successful API response could carry -- a malformed + // field inside an object, or a malformed top-level body (array, scalar, + // null). That is exactly MSW's `JsonBodyType`, so name it rather than + // `unknown`, which `HttpResponse.json` cannot serialize. + const serve = (body: JsonBodyType, requests: string[]) => { server.use( http.get(`${BASE_URL}/schedule_entries/4101`, () => { requests.push("GET"); diff --git a/typescript/tests/services/search.test.ts b/typescript/tests/services/search.test.ts index bb78331a3..f0d40e9a1 100644 --- a/typescript/tests/services/search.test.ts +++ b/typescript/tests/services/search.test.ts @@ -14,7 +14,7 @@ import type { BasecampClient } from "../../src/client.js"; // `api_search_result_template_path` special-cases. Imported rather than restated // so this cannot drift from the copy the other five SDKs and the conformance // runners assert against. -import searchResultsFixture from "../../../spec/fixtures/search/results.json"; +import searchResultsFixture from "../../../spec/fixtures/search/results.json" with { type: "json" }; const BASE_URL = "https://3.basecampapi.com/12345"; diff --git a/typescript/tests/services/subscriptions.test.ts b/typescript/tests/services/subscriptions.test.ts index 08dcda8c7..3bba2012e 100644 --- a/typescript/tests/services/subscriptions.test.ts +++ b/typescript/tests/services/subscriptions.test.ts @@ -44,8 +44,13 @@ describe("SubscriptionsService", () => { expect(subscription.subscribed).toBe(true); expect(subscription.count).toBe(3); - expect(subscription.subscribers).toHaveLength(3); - expect(subscription.subscribers[0].name).toBe("User One"); + // `subscribers` is optional in the generated schema. Assert presence + // first so a response that omits it fails here, then read through the + // narrowed local. + expect(subscription.subscribers).toBeDefined(); + const subscribers = subscription.subscribers!; + expect(subscribers).toHaveLength(3); + expect(subscribers[0].name).toBe("User One"); }); }); @@ -118,9 +123,12 @@ describe("SubscriptionsService", () => { }); expect(subscription.count).toBe(4); - expect(subscription.subscribers.map(s => s.id)).toContain(4); - expect(subscription.subscribers.map(s => s.id)).toContain(5); - expect(subscription.subscribers.map(s => s.id)).not.toContain(3); + // See the note in `get`: `subscribers` is optional. + expect(subscription.subscribers).toBeDefined(); + const subscribers = subscription.subscribers!; + expect(subscribers.map(s => s.id)).toContain(4); + expect(subscribers.map(s => s.id)).toContain(5); + expect(subscribers.map(s => s.id)).not.toContain(3); }); it("should work with only subscriptions", async () => { diff --git a/typescript/tests/services/todolists.test.ts b/typescript/tests/services/todolists.test.ts index ac62e0053..6087d0e9a 100644 --- a/typescript/tests/services/todolists.test.ts +++ b/typescript/tests/services/todolists.test.ts @@ -7,9 +7,9 @@ import { server } from "../setup.js"; import { createBasecampClient } from "../../src/client.js"; import { BasecampError } from "../../src/errors.js"; import type { BasecampClient } from "../../src/client.js"; -import todolistFixture from "../../../spec/fixtures/todolists/get.json"; -import groupFixture from "../../../spec/fixtures/todolist_groups/get.json"; -import groupListFixture from "../../../spec/fixtures/todolist_groups/list.json"; +import todolistFixture from "../../../spec/fixtures/todolists/get.json" with { type: "json" }; +import groupFixture from "../../../spec/fixtures/todolist_groups/get.json" with { type: "json" }; +import groupListFixture from "../../../spec/fixtures/todolist_groups/list.json" with { type: "json" }; import type { OperationInfo } from "../../src/hooks.js"; const BASE_URL = "https://3.basecampapi.com/12345"; diff --git a/typescript/tests/services/todos.test.ts b/typescript/tests/services/todos.test.ts index 395cc6b48..45e7dae8a 100644 --- a/typescript/tests/services/todos.test.ts +++ b/typescript/tests/services/todos.test.ts @@ -3,6 +3,7 @@ */ import { describe, it, expect, beforeEach } from "vitest"; import { http, HttpResponse } from "msw"; +import type { JsonBodyType } from "msw"; import { server } from "../setup.js"; import { createBasecampClient } from "../../src/client.js"; import { BasecampError } from "../../src/errors.js"; @@ -556,7 +557,11 @@ describe("TodosService", () => { const idLists = ["assignees", "completion_subscribers"] as const; // Serve a GET carrying `body` and a PUT that records that it happened. - const serve = (body: unknown, requests: string[]) => { + // `body` is typed as MSW's own response-body type rather than `unknown`: + // the malformed-*envelope* cases below deliberately serve arrays, scalars + // and null, so the parameter has to stay as wide as JSON itself -- but no + // wider, or the `HttpResponse.json` call cannot accept it. + const serve = (body: JsonBodyType, requests: string[]) => { server.use( http.get(`${BASE_URL}/todos/42`, () => { requests.push("GET"); diff --git a/typescript/tests/services/tools.test.ts b/typescript/tests/services/tools.test.ts index d1c0ff58e..866e18f86 100644 --- a/typescript/tests/services/tools.test.ts +++ b/typescript/tests/services/tools.test.ts @@ -13,11 +13,11 @@ import { server } from "../setup.js"; import { createBasecampClient } from "../../src/client.js"; import { BasecampError } from "../../src/errors.js"; import type { BasecampClient } from "../../src/client.js"; -import toolFixture from "../../../spec/fixtures/tools/get.json"; -import createdToolFixture from "../../../spec/fixtures/tools/create.json"; -import updatedToolFixture from "../../../spec/fixtures/tools/update.json"; -import disabledToolFixture from "../../../spec/fixtures/tools/disabled.json"; -import nestedVaultToolFixture from "../../../spec/fixtures/tools/nested_vault.json"; +import toolFixture from "../../../spec/fixtures/tools/get.json" with { type: "json" }; +import createdToolFixture from "../../../spec/fixtures/tools/create.json" with { type: "json" }; +import updatedToolFixture from "../../../spec/fixtures/tools/update.json" with { type: "json" }; +import disabledToolFixture from "../../../spec/fixtures/tools/disabled.json" with { type: "json" }; +import nestedVaultToolFixture from "../../../spec/fixtures/tools/nested_vault.json" with { type: "json" }; const BASE_URL = "https://3.basecampapi.com/12345"; diff --git a/typescript/tests/services/uploads.test.ts b/typescript/tests/services/uploads.test.ts index edb9ece23..c7fcd164d 100644 --- a/typescript/tests/services/uploads.test.ts +++ b/typescript/tests/services/uploads.test.ts @@ -11,7 +11,7 @@ import { server } from "../setup.js"; import { BasecampError } from "../../src/errors.js"; import { createBasecampClient } from "../../src/client.js"; // Sourced from the shared, coverage-guarded fixture (spec/fixtures/manifest.yaml) -import versionsFixture from "../../../spec/fixtures/uploads/versions.json"; +import versionsFixture from "../../../spec/fixtures/uploads/versions.json" with { type: "json" }; const BASE_URL = "https://3.basecampapi.com/12345"; @@ -19,6 +19,24 @@ const BASE_URL = "https://3.basecampapi.com/12345"; // type (the subclass lives in src/services/uploads-extensions.ts). type UploadsServiceT = ReturnType["uploads"]; +// House pattern (see tests/services/gauges.test.ts): a bare `.catch(e => e)` +// types the binding as `Upload | BasecampError`, so the error fields below +// don't typecheck. `rejection` turns an unexpected RESOLUTION into a failure +// rather than a silently-skipped assertion, and `asBasecampError` narrows only +// after asserting the class. +const rejection = async (promise: Promise): Promise => + promise.then( + () => { + throw new Error("expected the call to reject, but it resolved"); + }, + (error: unknown) => error, + ); + +const asBasecampError = (error: unknown): BasecampError => { + expect(error).toBeInstanceOf(BasecampError); + return error as BasecampError; +}; + describe("UploadsService", () => { let service: UploadsServiceT; @@ -164,17 +182,23 @@ describe("UploadsService", () => { }); it("should send all fields in request body", async () => { - let capturedBody: { - attachable_sgid?: string; - description?: string; - base_name?: string; - } | null = null; + // Held in an object, not a `let`: control-flow analysis cannot see the + // assignment inside the handler closure, so a `let ... = null` binding + // narrows to `null` and every field read below becomes `never`. The + // optional chaining still makes an unrun handler fail the assertions. + const captured: { + body?: { + attachable_sgid?: string; + description?: string; + base_name?: string; + }; + } = {}; server.use( http.post( `${BASE_URL}/vaults/1001/uploads.json`, async ({ request }) => { - capturedBody = (await request.json()) as { + captured.body = (await request.json()) as { attachable_sgid?: string; description?: string; base_name?: string; @@ -190,9 +214,9 @@ describe("UploadsService", () => { baseName: "custom-name", }); - expect(capturedBody?.attachable_sgid).toBe("test-sgid"); - expect(capturedBody?.description).toBe("

Description

"); - expect(capturedBody?.base_name).toBe("custom-name"); + expect(captured.body?.attachable_sgid).toBe("test-sgid"); + expect(captured.body?.description).toBe("

Description

"); + expect(captured.body?.base_name).toBe("custom-name"); }); // Client-side validation short-circuits before any HTTP call. No MSW handler @@ -229,12 +253,13 @@ describe("UploadsService", () => { }); it("should send updated fields in request body", async () => { - let capturedBody: { description?: string; base_name?: string } | null = - null; + // Object-held for the same reason as above. + const captured: { body?: { description?: string; base_name?: string } } = + {}; server.use( http.put(`${BASE_URL}/uploads/7001`, async ({ request }) => { - capturedBody = (await request.json()) as { + captured.body = (await request.json()) as { description?: string; base_name?: string; }; @@ -251,8 +276,8 @@ describe("UploadsService", () => { baseName: "renamed-file", }); - expect(capturedBody?.description).toBe("New description"); - expect(capturedBody?.base_name).toBe("renamed-file"); + expect(captured.body?.description).toBe("New description"); + expect(captured.body?.base_name).toBe("renamed-file"); }); }); @@ -443,11 +468,10 @@ describe("UploadsService", () => { }), ); - const error = await service - .createVersion(7001, { attachableSgid: "sgid-abc" }) - .catch((e) => e as BasecampError); + const error = asBasecampError( + await rejection(service.createVersion(7001, { attachableSgid: "sgid-abc" })), + ); - expect(error).toBeInstanceOf(BasecampError); expect(error.code).toBe("limit_exceeded"); expect(error.retryable).toBe(false); expect(error.message).toContain("storage limit"); diff --git a/typescript/tests/services/vaults.test.ts b/typescript/tests/services/vaults.test.ts index 64e11aac4..2195b7f3b 100644 --- a/typescript/tests/services/vaults.test.ts +++ b/typescript/tests/services/vaults.test.ts @@ -121,18 +121,22 @@ describe("VaultsService", () => { }); it("should send title in request body", async () => { - let capturedBody: { title?: string } | null = null; + // Held in an object rather than a bare `let`: TS's control-flow analysis + // cannot see the assignment inside the MSW handler closure, so a + // `let x: T | null = null` narrows to `null` at the assertion below and + // the property read becomes an error on `never`. + const captured: { body?: { title?: string } } = {}; server.use( http.post(`${BASE_URL}/vaults/1001/vaults.json`, async ({ request }) => { - capturedBody = (await request.json()) as { title?: string }; + captured.body = (await request.json()) as { title?: string }; return HttpResponse.json({ id: 1, title: "Test" }); }) ); await service.create(1001, { title: "My New Folder" }); - expect(capturedBody?.title).toBe("My New Folder"); + expect(captured.body?.title).toBe("My New Folder"); }); // Client-side validation short-circuits before any HTTP call. No MSW handler @@ -164,18 +168,20 @@ describe("VaultsService", () => { }); it("should send title in request body", async () => { - let capturedBody: { title?: string } | null = null; + // See the note in `create` above: a `let` assigned only inside the MSW + // handler closure narrows to `null` for the assertion. + const captured: { body?: { title?: string } } = {}; server.use( http.put(`${BASE_URL}/vaults/1001`, async ({ request }) => { - capturedBody = (await request.json()) as { title?: string }; + captured.body = (await request.json()) as { title?: string }; return HttpResponse.json({ id: 1001, title: "Updated" }); }) ); await service.update(1001, { title: "Updated Title" }); - expect(capturedBody?.title).toBe("Updated Title"); + expect(captured.body?.title).toBe("Updated Title"); }); }); }); diff --git a/typescript/tsconfig.test.json b/typescript/tsconfig.test.json new file mode 100644 index 000000000..b1cf10bde --- /dev/null +++ b/typescript/tsconfig.test.json @@ -0,0 +1,31 @@ +// Typecheck-only project covering the tests and the generator scripts +// alongside src/. `npm run typecheck` runs it after the build project. +// +// tsconfig.json is the *build* project: it emits declarations under +// rootDir: "src", so tests cannot join it without landing in dist/. This +// project turns emit off, widens rootDir to the package, and — critically — +// resets `exclude`, since an inherited "tests" entry silently drops every test +// file from the program even when `include` names them. That inherited +// exclusion is what let issue #737 ship: four generated methods declared a +// return type the runtime contradicted, and no job ever typechecked a caller. +// +// noUncheckedIndexedAccess is deliberately off here. It stays on for the +// shipped surface (the build project still checks src/ with it), but in test +// code it only buys `calls[0]!` ceremony: an undefined index fails the +// assertion on the next line anyway. +// +// `lib` is ES2023 because that is what the code in this project actually runs +// on: the scripts run under tsx on Node >= 22.12 (see package.json engines) +// and already use ES2023 array methods. The shipped surface keeps the narrower +// ES2022 lib of the build project, which still checks src/ on its own. +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "noEmit": true, + "rootDir": ".", + "lib": ["ES2023"], + "noUncheckedIndexedAccess": false + }, + "include": ["src/**/*", "tests/**/*", "scripts/**/*"], + "exclude": ["node_modules", "dist"] +} From a5d9eb4c1e6ebcd99b97553b9fcbbb56c68830ad Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 16 Aug 2026 22:03:45 -0700 Subject: [PATCH 2/5] Keep the ListResult wrapper when a paginated entity has no alias `buildReturnType()` resolved element type names only through the hand-maintained `TYPE_ALIASES` map. `Gauge`, `GaugeNeedle`, `SearchResult` and `QuestionReminder` are not in it, so `getEntityTypeName()` returned null and the function fell through to its "fallback to schema ref" line -- which returned the bare `*ResponseContent` array and dropped `ListResult<>` entirely, even with `op.returnsArray && op.hasPagination` both true. The runtime object is a `ListResult` in all four cases: `requestPaginated` builds one, and the JSDoc on all four already promises ".meta.totalCount". So the docs and the runtime agreed with each other and only the signature disagreed, making `result.meta.totalCount` a type error on four methods where every sibling allows it. Fixes #737. Adding the four missing aliases would have fixed four symptoms and left the mechanism intact, waiting for the next unaliased paginated entity. Instead the fallback is now pagination-aware: a paginated array keeps its wrapper and only the element name degrades, to the item's own schema ref -- which is a perfectly good element type, just not a friendly one. The same lookup had a second hole one level down. Wrapped pagination (a list under a key of an object response) spelled an unaliased entity `ListResult`, in two places that have to agree: the declared return type and the `requestPaginatedWrapped` type argument, each resolving the entity with its own copy of the expression. Both now call one `buildPaginationElementType()` helper, which resolves the array form and the wrapped form the same way and reaches `unknown` only for a schema that names no element at all. No generated output changes there today -- the one wrapped operation's entity is aliased -- but the two sites can no longer drift apart. `tests/types/paginated-returns.test-d.ts` pins the contract in the type system, where the previous commit's typecheck can see it: each of the four methods returns a `ListResult` carrying `.meta`, with a concrete element type. It covers the wrapped shape and the already-aliased `TodosService#list` too, and a negative control asserts a single-entity method does NOT satisfy the predicate, so the assertions cannot pass vacuously. Reverting this commit's generator change and regenerating fails it with 8 TS2344s; forcing the wrapped element back to `unknown` fails exactly the one assertion that covers it. `tests/services/gauges.test.ts` drops the `metaOf()` helper that asserted `instanceof ListResult` at runtime to reach `.meta` past the wrong signature. It now reads `gauges.meta` directly, with one explicit `toBeInstanceOf` assertion left in the first test to pin the runtime class behind the type. --- typescript/scripts/generate-services.ts | 46 +++++++-- typescript/src/generated/services/checkins.ts | 2 +- typescript/src/generated/services/gauges.ts | 4 +- typescript/src/generated/services/search.ts | 2 +- typescript/tests/services/gauges.test.ts | 41 +++----- .../tests/types/paginated-returns.test-d.ts | 99 +++++++++++++++++++ 6 files changed, 157 insertions(+), 37 deletions(-) create mode 100644 typescript/tests/types/paginated-returns.test-d.ts diff --git a/typescript/scripts/generate-services.ts b/typescript/scripts/generate-services.ts index fca9c241e..7c802d71b 100644 --- a/typescript/scripts/generate-services.ts +++ b/typescript/scripts/generate-services.ts @@ -1319,9 +1319,7 @@ function generateMethod(op: ParsedOperation, serviceName: string): string[] { } else if (isPaginated) { lines.push(` return this.requestPaginated(`); } else if (isWrappedPaginated) { - const entitySchema = findUnderlyingEntitySchema(op.responseSchemaRef || "", op.paginationKey); - const entityName = entitySchema && TYPE_ALIASES[entitySchema] ? TYPE_ALIASES[entitySchema][0] : "unknown"; - lines.push(` return this.requestPaginatedWrapped<"${op.paginationKey}", ${entityName}>(`); + lines.push(` return this.requestPaginatedWrapped<"${op.paginationKey}", ${buildPaginationElementType(op)}>(`); } else { lines.push(` const response = await this.request(`); } @@ -1478,6 +1476,36 @@ function buildMethodSignature(op: ParsedOperation, resourceName: string): { }; } +/** + * The element type to put inside `ListResult<...>` for a paginated operation. + * + * Both paginated shapes resolve the same way, one level apart: a bare array + * response takes its own `items`, a wrapped response takes the `items` of the + * property named by the pagination key. The name is the friendly alias when the + * entity has one, otherwise the item's own schema ref — degrading to a schema + * ref keeps the element concrete where a missing TYPE_ALIASES entry used to + * cost the whole `ListResult` wrapper (array form) or the element type + * (wrapped form, which spelled `unknown`). Only a schema that names no element + * at all reaches `unknown` now. + * + * Callers must use this for BOTH the declared return type and the + * `requestPaginatedWrapped` type argument, which have to agree. + */ +function buildPaginationElementType(op: ParsedOperation): string { + const responseSchema = op.responseSchemaRef ? globalSchemas[op.responseSchemaRef] : undefined; + const listSchema = op.returnsArray + ? responseSchema + : op.paginationKey + ? responseSchema?.properties?.[op.paginationKey] + : undefined; + + const itemRef = listSchema?.items?.$ref ? resolveRef(listSchema.items.$ref) : ""; + if (!itemRef) return "unknown"; + + const alias = TYPE_ALIASES[itemRef]; + return alias ? alias[0] : `components["schemas"]["${itemRef}"]`; +} + function buildReturnType(op: ParsedOperation, serviceName: string): string { if (op.returnsVoid) return "void"; @@ -1490,9 +1518,7 @@ function buildReturnType(op: ParsedOperation, serviceName: string): string { const parts: string[] = []; for (const [propName, propSchema] of Object.entries(schema.properties)) { if (propName === op.paginationKey) { - const entitySchema = findUnderlyingEntitySchema(op.responseSchemaRef, op.paginationKey); - const entityName = entitySchema && TYPE_ALIASES[entitySchema] ? TYPE_ALIASES[entitySchema][0] : "unknown"; - parts.push(`${propName}: ListResult<${entityName}>`); + parts.push(`${propName}: ListResult<${buildPaginationElementType(op)}>`); } else { const propType = propSchema.$ref ? (() => { @@ -1514,7 +1540,13 @@ function buildReturnType(op: ParsedOperation, serviceName: string): string { } return op.returnsArray ? `${entityName}[]` : entityName; } - // Fallback to schema ref + // No friendly name: the entity has no TYPE_ALIASES entry. Fall back to the + // response schema ref — but a paginated array still comes back as a + // ListResult at runtime (requestPaginated builds one), so the wrapper has to + // survive the fallback. Only the element name degrades, to its schema ref. + if (op.returnsArray && op.hasPagination) { + return `ListResult<${buildPaginationElementType(op)}>`; + } return `components["schemas"]["${op.responseSchemaRef}"]`; } diff --git a/typescript/src/generated/services/checkins.ts b/typescript/src/generated/services/checkins.ts index 6e4945c81..f04ff9190 100644 --- a/typescript/src/generated/services/checkins.ts +++ b/typescript/src/generated/services/checkins.ts @@ -135,7 +135,7 @@ export class CheckinsService extends BaseService { * const result = await client.checkins.reminders(); * ``` */ - async reminders(options?: RemindersCheckinOptions): Promise { + async reminders(options?: RemindersCheckinOptions): Promise> { return this.requestPaginated( { service: "Checkins", diff --git a/typescript/src/generated/services/gauges.ts b/typescript/src/generated/services/gauges.ts index 81277ce5a..1001a0959 100644 --- a/typescript/src/generated/services/gauges.ts +++ b/typescript/src/generated/services/gauges.ts @@ -214,7 +214,7 @@ export class GaugesService extends BaseService { * const filtered = await client.gauges.listGaugeNeedles(123, { page: 1 }); * ``` */ - async listGaugeNeedles(projectId: number, options?: ListGaugeNeedlesGaugeOptions): Promise { + async listGaugeNeedles(projectId: number, options?: ListGaugeNeedlesGaugeOptions): Promise> { return this.requestPaginated( { service: "Gauges", @@ -286,7 +286,7 @@ export class GaugesService extends BaseService { * const filtered = await client.gauges.listGauges({ bucketIds: "example" }); * ``` */ - async listGauges(options?: ListGaugesGaugeOptions): Promise { + async listGauges(options?: ListGaugesGaugeOptions): Promise> { return this.requestPaginated( { service: "Gauges", diff --git a/typescript/src/generated/services/search.ts b/typescript/src/generated/services/search.ts index b8772e50c..60157f42f 100644 --- a/typescript/src/generated/services/search.ts +++ b/typescript/src/generated/services/search.ts @@ -74,7 +74,7 @@ export class SearchService extends BaseService { * const result = await client.search.search("q"); * ``` */ - async search(q: string, options?: SearchSearchOptions): Promise { + async search(q: string, options?: SearchSearchOptions): Promise> { return this.requestPaginated( { service: "Search", diff --git a/typescript/tests/services/gauges.test.ts b/typescript/tests/services/gauges.test.ts index 72040e029..e6deb0edc 100644 --- a/typescript/tests/services/gauges.test.ts +++ b/typescript/tests/services/gauges.test.ts @@ -17,7 +17,6 @@ import { server } from "../setup.js"; import { createBasecampClient } from "../../src/client.js"; import { BasecampError } from "../../src/errors.js"; import { ListResult } from "../../src/pagination.js"; -import type { ListMeta } from "../../src/pagination.js"; import type { BasecampClient } from "../../src/client.js"; import type { CreateGaugeNeedleGaugeRequest, @@ -47,20 +46,6 @@ const asBasecampError = (error: unknown): BasecampError => { return error as BasecampError; }; -// Both gauge list methods return a ListResult at runtime — requestPaginated -// builds one — but their generated signatures declare the bare -// `List*ResponseContent` ARRAY, so `.meta` is invisible to the compiler. Nearly -// every other generated list method declares `Promise>`; -// ListGauges, ListGaugeNeedles, Search and Checkins#reminders are the four that -// don't. The repo's `tsc --noEmit` excludes tests, so this reads clean today -// either way; the helper asserts the runtime class first, so the pagination -// contract below is pinned against what the object IS, not what the (currently -// under-specified) return type claims. Reported, not fixed — the fix belongs in -// the service generator, not in a test. -const metaOf = (list: unknown): ListMeta => { - expect(list).toBeInstanceOf(ListResult); - return (list as ListResult).meta; -}; describe("GaugesService", () => { let client: BasecampClient; @@ -99,8 +84,12 @@ describe("GaugesService", () => { expect(gauge.bucket?.id).toBe(gaugeFixture.bucket.id); expect(gauge.bucket?.type).toBe("Project"); expect(gauges[1]!.id).toBe(2); - expect(metaOf(gauges).totalCount).toBe(2); - expect(metaOf(gauges).truncated).toBe(false); + // The declared return type says ListResult, so `.meta` below compiles; + // this pins the runtime class behind it (an Array subclass, so plain + // array assertions keep working either way). + expect(gauges).toBeInstanceOf(ListResult); + expect(gauges.meta.totalCount).toBe(2); + expect(gauges.meta.truncated).toBe(false); }); // The filter is spelled `bucket_ids` on the wire (snake_case), not @@ -150,8 +139,8 @@ describe("GaugesService", () => { expect(requested).toEqual(["3"]); expect(gauges).toHaveLength(2); - expect(metaOf(gauges).totalCount).toBe(9); - expect(metaOf(gauges).truncated).toBe(true); + expect(gauges.meta.totalCount).toBe(9); + expect(gauges.meta.truncated).toBe(true); }); it("follows Link headers across pages when no page is pinned", async () => { @@ -178,8 +167,8 @@ describe("GaugesService", () => { expect(requested).toEqual(["(none)", "2"]); expect(gauges.map((g) => g.id)).toEqual([1, 2, 3]); - expect(metaOf(gauges).totalCount).toBe(3); - expect(metaOf(gauges).truncated).toBe(false); + expect(gauges.meta.totalCount).toBe(3); + expect(gauges.meta.truncated).toBe(false); }); // ListGauges lists ForbiddenError/UnauthorizedError/RateLimitError/ @@ -223,8 +212,8 @@ describe("GaugesService", () => { expect(needle.position).toBe(72); // A needle hangs off its gauge, so the recording parent is the Gauge. expect(needle.parent?.id).toBe(needleFixture.parent.id); - expect(metaOf(needles).totalCount).toBe(2); - expect(metaOf(needles).truncated).toBe(false); + expect(needles.meta.totalCount).toBe(2); + expect(needles.meta.truncated).toBe(false); }); it("selects exactly one page and reports the unfollowed next link", async () => { @@ -248,8 +237,8 @@ describe("GaugesService", () => { expect(requested).toEqual(["2"]); expect(needles).toHaveLength(1); - expect(metaOf(needles).totalCount).toBe(5); - expect(metaOf(needles).truncated).toBe(true); + expect(needles.meta.totalCount).toBe(5); + expect(needles.meta.truncated).toBe(true); }); it("follows Link headers across pages when no page is pinned", async () => { @@ -277,7 +266,7 @@ describe("GaugesService", () => { expect(requested).toEqual(["(none)", "2"]); expect(needles.map((n) => n.id)).toEqual([1, 2, 3]); - expect(metaOf(needles).truncated).toBe(false); + expect(needles.meta.truncated).toBe(false); }); it("maps a 404 on an unknown project to not_found", async () => { diff --git a/typescript/tests/types/paginated-returns.test-d.ts b/typescript/tests/types/paginated-returns.test-d.ts new file mode 100644 index 000000000..d34c61422 --- /dev/null +++ b/typescript/tests/types/paginated-returns.test-d.ts @@ -0,0 +1,99 @@ +/** + * Type-level assertions for the return types of paginated service methods. + * + * Every generated method that calls `requestPaginated` resolves to a + * `ListResult` at runtime — an Array subclass carrying `.meta` — and its + * JSDoc promises ".meta.totalCount". This file pins that promise in the type + * system, so a generator change that drops the wrapper (issue #737: four + * methods declared the bare `*ResponseContent` array because their entity had + * no `TYPE_ALIASES` entry) fails the typecheck instead of shipping. + * + * There is no runtime here: the assertions are checked by `tsc`, via + * `tsconfig.test.json` / `make ts-typecheck`. The `.test-d.ts` suffix keeps + * vitest (`include: tests/**\/*.test.ts`) from collecting a file with no tests + * in it. + */ +import type { ListMeta, ListResult } from "../../src/pagination.js"; +import type { CheckinsService } from "../../src/generated/services/checkins.js"; +import type { GaugesService } from "../../src/generated/services/gauges.js"; +import type { ReportsService } from "../../src/generated/services/reports.js"; +import type { SearchService } from "../../src/generated/services/search.js"; +import type { TodosService } from "../../src/generated/services/todos.js"; + +/** Compiles only when `T` is `true`; anything else is a TS2344 constraint error. */ +type Expect = T; + +type AsyncMethod = (...args: never[]) => Promise; + +/** What awaiting a service method yields. */ +type Returned = Awaited>; + +/** + * `ListResult` extends `Array`, so this predicate has to be read in the + * narrow direction: a plain `Gauge[]` lacks `.meta` and therefore does NOT + * extend `ListResult`, while any `ListResult` does. + */ +type IsListResult = Returned extends ListResult ? true : false; + +/** The list carries pagination metadata of the declared shape. */ +type HasListMeta = Returned extends { meta: ListMeta } ? true : false; + +/** + * The element type resolved to something concrete. `unknown extends T` is also + * true for `any`, so both degenerate spellings are caught — this is what guards + * against a `ListResult` regression when an entity is unaliased. + */ +type ElementIsResolved = unknown extends T ? false : true; + +// ----------------------------------------------------------------------------- +// The four methods from #737: paginated arrays whose entity has no TYPE_ALIASES +// entry (Gauge, GaugeNeedle, SearchResult, QuestionReminder). +// ----------------------------------------------------------------------------- + +export type ListGaugesIsListResult = Expect>; +export type ListGaugesHasMeta = Expect>; +export type ListGaugesElement = Expect[number]>>; + +export type ListGaugeNeedlesIsListResult = Expect>; +export type ListGaugeNeedlesHasMeta = Expect>; +export type ListGaugeNeedlesElement = Expect< + ElementIsResolved[number]> +>; + +export type SearchIsListResult = Expect>; +export type SearchHasMeta = Expect>; +export type SearchElement = Expect[number]>>; + +export type RemindersIsListResult = Expect>; +export type RemindersHasMeta = Expect>; +export type RemindersElement = Expect[number]>>; + +// ----------------------------------------------------------------------------- +// A method whose entity IS aliased, so the wrapper never depended on the +// fallback. Present so a regression that reaches every list method, not just +// the unaliased ones, is visible here too. +// ----------------------------------------------------------------------------- + +export type TodosListIsListResult = Expect>; +export type TodosListElement = Expect[number]>>; + +// ----------------------------------------------------------------------------- +// Wrapped pagination: the list lives under a key of an object response. Its +// element type is resolved through the same TYPE_ALIASES lookup, whose miss +// spelled `ListResult` before #737. +// ----------------------------------------------------------------------------- + +type PersonProgressEvents = Returned["events"]; + +export type PersonProgressIsListResult = Expect ? true : false>; +export type PersonProgressElement = Expect>; + +// ----------------------------------------------------------------------------- +// Negative control: the predicate has to be able to say "no". A single-entity +// method must NOT satisfy it — if this line ever compiles as `true`, the +// assertions above are vacuous. +// ----------------------------------------------------------------------------- + +export type SingleGaugeNeedleIsNotListResult = Expect< + IsListResult extends false ? true : false +>; From 6f6c3a0ccc727301f23f95b994751444fa535039 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 16 Aug 2026 22:13:42 -0700 Subject: [PATCH 3/5] Cover the wrapped alias-miss the type assertions structurally cannot reach The type-level assertions pin the four real operations that hit the array-form alias miss. They cannot reach the wrapped form: the one wrapped-pagination operation in the spec carries an aliased entity, so no generated signature would move if that branch regressed to `unknown` again. Raised in review. Drive `buildReturnType` directly instead, following the generator-regression pattern `tests/generator/example-value.test.ts` already establishes (`setSchemas` + an exported function). Six cases: aliased and unaliased for both the array and the wrapped form, the unpaginated array that must still fall back to its schema ref, and the floor where the items name no schema at all and only `ListResult` is left. The unaliased entity is a fictional `WidgetThing` rather than a real one, so a later TYPE_ALIASES addition cannot quietly turn a miss case into a hit case. Reverting the array fallback fails 2 of the 6; restoring the wrapped alias-miss to `unknown` fails exactly 1 -- the case that has no other cover. --- typescript/scripts/generate-services.ts | 4 +- .../generator/pagination-return-type.test.ts | 163 ++++++++++++++++++ 2 files changed, 165 insertions(+), 2 deletions(-) create mode 100644 typescript/tests/generator/pagination-return-type.test.ts diff --git a/typescript/scripts/generate-services.ts b/typescript/scripts/generate-services.ts index 7c802d71b..5652086cb 100644 --- a/typescript/scripts/generate-services.ts +++ b/typescript/scripts/generate-services.ts @@ -1776,5 +1776,5 @@ if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) } // Exported for generator regression tests. -export { generateExampleValue, setSchemas }; -export type { Schema }; +export { generateExampleValue, setSchemas, buildReturnType }; +export type { Schema, ParsedOperation }; diff --git a/typescript/tests/generator/pagination-return-type.test.ts b/typescript/tests/generator/pagination-return-type.test.ts new file mode 100644 index 000000000..27d4b2ad1 --- /dev/null +++ b/typescript/tests/generator/pagination-return-type.test.ts @@ -0,0 +1,163 @@ +import { describe, it, expect, beforeEach } from "vitest"; +import { buildReturnType, setSchemas, type ParsedOperation, type Schema } from "../../scripts/generate-services.js"; + +// Regression coverage for the return type of paginated operations (#737). +// +// `buildReturnType` resolves a friendly element name through the hand-maintained +// TYPE_ALIASES map, and everything below is about what happens when that lookup +// MISSES. Before the fix a missed lookup fell through to the raw response schema +// ref and dropped the `ListResult<>` wrapper entirely for the array form, and +// spelled `ListResult` for the wrapped form. +// +// The type-level assertions in tests/types/paginated-returns.test-d.ts pin the +// four real operations that hit the array miss. They CANNOT reach the wrapped +// miss: every wrapped-pagination operation in the spec today (there is one) +// carries an aliased entity, so no generated signature would change if that +// branch regressed. These unit tests drive the function directly, so the branch +// is covered whether or not the spec ever grows such an operation. +// +// `WidgetThing` is deliberately fictional: an entity name that is not in +// TYPE_ALIASES and cannot quietly acquire an entry later, which would otherwise +// turn a miss case into a hit case without anyone noticing. + +const schemas: Record = { + // Bare array response, entity absent from TYPE_ALIASES — the #737 shape. + ListWidgetsResponseContent: { + type: "array", + items: { $ref: "#/components/schemas/WidgetThing" }, + }, + // Bare array response, entity present in TYPE_ALIASES. + ListTodosResponseContent: { + type: "array", + items: { $ref: "#/components/schemas/Todo" }, + }, + // Bare array response whose items name no schema at all. + ListAnonymousResponseContent: { + type: "array", + items: { type: "object" }, + }, + // Wrapped pagination, entity absent from TYPE_ALIASES. + WidgetReportResponseContent: { + type: "object", + properties: { + person: { $ref: "#/components/schemas/Person" }, + widgets: { type: "array", items: { $ref: "#/components/schemas/WidgetThing" } }, + }, + }, + // Wrapped pagination, entity present in TYPE_ALIASES — the shape the spec has. + TimelineReportResponseContent: { + type: "object", + properties: { + person: { $ref: "#/components/schemas/Person" }, + events: { type: "array", items: { $ref: "#/components/schemas/TimelineEvent" } }, + }, + }, + WidgetThing: { type: "object", properties: { id: { type: "integer" } } }, + Todo: { type: "object", properties: { id: { type: "integer" } } }, + Person: { type: "object", properties: { id: { type: "integer" } } }, + TimelineEvent: { type: "object", properties: { id: { type: "integer" } } }, +}; + +const operation = (overrides: Partial): ParsedOperation => ({ + operationId: "ListWidgets", + methodName: "listWidgets", + httpMethod: "GET", + path: "/widgets.json", + description: "List widgets.", + pathParams: [], + queryParams: [], + bodyProperties: [], + bodyRequired: false, + returnsArray: false, + returnsVoid: false, + isMutation: false, + resourceType: "widget", + hasPagination: false, + serviceName: "Widgets", + ...overrides, +}); + +describe("buildReturnType — paginated operations", () => { + beforeEach(() => { + setSchemas(schemas); + }); + + describe("bare array responses", () => { + it("wraps an aliased entity in ListResult under its friendly name", () => { + const returnType = buildReturnType( + operation({ responseSchemaRef: "ListTodosResponseContent", returnsArray: true, hasPagination: true }), + "Todos", + ); + + expect(returnType).toBe("ListResult"); + }); + + // The #737 regression: this returned the bare + // `components["schemas"]["ListWidgetsResponseContent"]` array, so `.meta` + // was invisible to the compiler on a value that carries it at runtime. + it("keeps the ListResult wrapper when the entity has no alias", () => { + const returnType = buildReturnType( + operation({ responseSchemaRef: "ListWidgetsResponseContent", returnsArray: true, hasPagination: true }), + "Widgets", + ); + + expect(returnType).toBe('ListResult'); + }); + + // The wrapper is tied to pagination, not to arrays: an unpaginated array + // returns no ListResult and must still fall back to the schema ref. + it("leaves an unpaginated array as its response schema ref", () => { + const returnType = buildReturnType( + operation({ responseSchemaRef: "ListWidgetsResponseContent", returnsArray: true, hasPagination: false }), + "Widgets", + ); + + expect(returnType).toBe('components["schemas"]["ListWidgetsResponseContent"]'); + }); + + // The floor. No schema names the element, so there is nothing concrete to + // put inside the wrapper — but the wrapper itself still survives, because + // requestPaginated builds a ListResult either way. + it("falls back to ListResult when the items name no schema", () => { + const returnType = buildReturnType( + operation({ responseSchemaRef: "ListAnonymousResponseContent", returnsArray: true, hasPagination: true }), + "Widgets", + ); + + expect(returnType).toBe("ListResult"); + }); + }); + + describe("wrapped pagination", () => { + it("wraps an aliased entity in ListResult under its friendly name", () => { + const returnType = buildReturnType( + operation({ + responseSchemaRef: "TimelineReportResponseContent", + returnsArray: false, + hasPagination: true, + paginationKey: "events", + }), + "Reports", + ); + + expect(returnType).toBe("{ person: Person; events: ListResult }"); + }); + + // Unreachable from the current spec, which is exactly why it is here: the + // alias miss used to spell `ListResult` and no generated signature + // would move if it did so again. + it("uses the item's schema ref when the entity has no alias", () => { + const returnType = buildReturnType( + operation({ + responseSchemaRef: "WidgetReportResponseContent", + returnsArray: false, + hasPagination: true, + paginationKey: "widgets", + }), + "Reports", + ); + + expect(returnType).toBe('{ person: Person; widgets: ListResult }'); + }); + }); +}); From 3a7a50827565ed4eea54bd9256c2b943d28f5446 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 16 Aug 2026 22:41:42 -0700 Subject: [PATCH 4/5] Record why skipLibCheck does not reach the type assertions Two review bots independently read `paginated-returns.test-d.ts` as a declaration file and concluded the assertions are inert under the inherited `skipLibCheck: true`. It ends in `-d.ts`, not `.d.ts`, so TypeScript checks it like any other source -- flipping an assertion to a knowingly false one reports TS2344 on that line, and the branch's red proof is 8 such errors in that file. Their proposed remedy is worse than the disease: `skipLibCheck: false` fails on 22 errors inside @mswjs/interceptors' browser `.d.mts` and adds ~20s per run. Write both facts where the next reader looks -- the file header and the config -- with the recipe to re-verify in one command, so the next reader checks instead of re-opening. --- typescript/tests/types/paginated-returns.test-d.ts | 9 +++++++++ typescript/tsconfig.test.json | 7 +++++++ 2 files changed, 16 insertions(+) diff --git a/typescript/tests/types/paginated-returns.test-d.ts b/typescript/tests/types/paginated-returns.test-d.ts index d34c61422..1364aa266 100644 --- a/typescript/tests/types/paginated-returns.test-d.ts +++ b/typescript/tests/types/paginated-returns.test-d.ts @@ -12,6 +12,15 @@ * `tsconfig.test.json` / `make ts-typecheck`. The `.test-d.ts` suffix keeps * vitest (`include: tests/**\/*.test.ts`) from collecting a file with no tests * in it. + * + * Despite that suffix this is NOT a declaration file: TypeScript treats a file + * as one only when the name ends in `.d.ts`, and this ends in `-d.ts`. The + * `skipLibCheck: true` inherited by `tsconfig.test.json` therefore never + * reaches it. Two review bots have read it the other way, so check rather than + * argue: flip any `Expect<...>` below to a knowingly false one and + * `npx tsc -p tsconfig.test.json --noEmit` reports `TS2344` on that line. + * Setting `skipLibCheck: false` to "make sure" is not the fix -- it fails on + * 22 errors inside @mswjs/interceptors' browser `.d.mts` and costs ~20s. */ import type { ListMeta, ListResult } from "../../src/pagination.js"; import type { CheckinsService } from "../../src/generated/services/checkins.js"; diff --git a/typescript/tsconfig.test.json b/typescript/tsconfig.test.json index b1cf10bde..3fe75453d 100644 --- a/typescript/tsconfig.test.json +++ b/typescript/tsconfig.test.json @@ -14,6 +14,13 @@ // code it only buys `calls[0]!` ceremony: an undefined index fails the // assertion on the next line anyway. // +// `skipLibCheck: true` is inherited and stays. It does not weaken the type +// assertions in tests/types/paginated-returns.test-d.ts: that file ends in +// `-d.ts`, not `.d.ts`, so TypeScript checks it like any other source (flip an +// assertion to a false one and tsc reports TS2344 on it). Turning the flag off +// is not free -- it fails on 22 errors inside @mswjs/interceptors' browser +// `.d.mts` and adds ~20s to every run. +// // `lib` is ES2023 because that is what the code in this project actually runs // on: the scripts run under tsx on Node >= 22.12 (see package.json engines) // and already use ES2023 array methods. The shipped surface keeps the narrower From 6908bb613d69f0ec00989c0ae51527da2f73b972 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 17 Aug 2026 11:04:37 -0700 Subject: [PATCH 5/5] Lock the two halves of a wrapped-paginated signature together MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The claim that the declared return type and the `requestPaginatedWrapped` type argument can no longer drift apart was unenforced: both call the same `buildPaginationElementType`, but only the first is asserted anywhere. Regressing the second to `unknown` and regenerating leaves `make ts-check` entirely green — 1525 tests, both typecheck projects, and the drift check, which passes because the committed output was regenerated to match. The generated method casts its result to the separately built return type, so the disagreement never reaches the compiler; `paginated-returns.test-d.ts` cannot reach it either, since it pins the four operations that hit the array miss. Export `generateMethod` and assert the emitted method names the same element as the declared return type, for the aliased hit and the unaliased miss. Both cases fail against that mutation. --- typescript/scripts/generate-services.ts | 7 +++- .../generator/pagination-return-type.test.ts | 39 ++++++++++++++++++- 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/typescript/scripts/generate-services.ts b/typescript/scripts/generate-services.ts index 5652086cb..10f0c2473 100644 --- a/typescript/scripts/generate-services.ts +++ b/typescript/scripts/generate-services.ts @@ -1775,6 +1775,9 @@ if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) main(); } -// Exported for generator regression tests. -export { generateExampleValue, setSchemas, buildReturnType }; +// Exported for generator regression tests. `generateMethod` is here because +// the declared return type is only half of a wrapped-paginated signature — +// nothing but the emitted method shows the `requestPaginatedWrapped` type +// argument, and the two have to name the same element. +export { generateExampleValue, setSchemas, buildReturnType, generateMethod }; export type { Schema, ParsedOperation }; diff --git a/typescript/tests/generator/pagination-return-type.test.ts b/typescript/tests/generator/pagination-return-type.test.ts index 27d4b2ad1..86b3a04ed 100644 --- a/typescript/tests/generator/pagination-return-type.test.ts +++ b/typescript/tests/generator/pagination-return-type.test.ts @@ -1,5 +1,11 @@ import { describe, it, expect, beforeEach } from "vitest"; -import { buildReturnType, setSchemas, type ParsedOperation, type Schema } from "../../scripts/generate-services.js"; +import { + buildReturnType, + generateMethod, + setSchemas, + type ParsedOperation, + type Schema, +} from "../../scripts/generate-services.js"; // Regression coverage for the return type of paginated operations (#737). // @@ -159,5 +165,36 @@ describe("buildReturnType — paginated operations", () => { expect(returnType).toBe('{ person: Person; widgets: ListResult }'); }); + + // A wrapped-paginated signature is emitted in two places that must name the + // same element: the declared return type above, and the + // `requestPaginatedWrapped` type argument inside the method body. + // `buildReturnType` cannot see the second one, and neither can anything + // else in the repo — verified by mutation. Regressing that argument to + // `unknown` and regenerating leaves `make ts-check` entirely green: drift + // passes (the committed output was regenerated to match), both typecheck + // projects pass (the generated method casts the result to its separately + // built return type, so the disagreement never surfaces), and all tests + // pass. `paginated-returns.test-d.ts` cannot reach it either — it pins the + // four operations that hit the ARRAY miss. + // + // So the agreement is asserted here, on the emitted method, for both the + // hit and the miss. + it.each([ + { ref: "TimelineReportResponseContent", key: "events", element: "TimelineEvent" }, + { ref: "WidgetReportResponseContent", key: "widgets", element: 'components["schemas"]["WidgetThing"]' }, + ])("emits $element as both the declared element and the paginated type argument", ({ ref, key, element }) => { + const op = operation({ + responseSchemaRef: ref, + returnsArray: false, + hasPagination: true, + paginationKey: key, + }); + + expect(generateMethod(op, "Reports").join("\n")).toContain( + `return this.requestPaginatedWrapped<"${key}", ${element}>(`, + ); + expect(buildReturnType(op, "Reports")).toContain(`${key}: ListResult<${element}>`); + }); }); });