feat: remove batch support and rename API v3 → v1#2
Conversation
Comms no longer exposes a /batch endpoint, so drop the BatchBuilder,
the batch types, the api.batch() entry point, and the { batch: true }
overload on every client method. Each client method now has a single
signature returning Promise<T> instead of the prior 3-overload pattern.
Hard fork: rename API_VERSIONS to ['v1'], drop the deprecated
API_BASE_URI / API_VERSION constants, update JSDoc and tests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
amix
force-pushed
the
amix/remove-batch-support
branch
from
cf55f45 to
0c67b65
Compare
doistbot
left a comment
doistbot
left a comment
There was a problem hiding this comment.
This PR effectively streamlines the SDK for the Comms hard fork by stripping out deprecated batching support, simplifying client signatures, and updating the API version to v1. These cleanup efforts significantly reduce complexity and nicely align the codebase with the current reality of the API. There are a few opportunities for further refinement noted, specifically around DRYing up shared request helpers, removing residual version configuration plumbing, leveraging shared URL builders in tests, and adjusting a few localized typing and serialization details to better adhere to repository guidelines.
Per Doistbot review on #2: - Drop dead ApiVersion / DEFAULT_API_VERSION / API_VERSIONS plumbing now that the SDK only supports v1. `getCommsBaseUri()` and `BaseClient.getBaseUri()` no longer take a version argument; the ClientConfig.version field is gone. - reactionTarget() now returns camelCase keys; transport snake-cases on the wire, matching the rest of the clients. - add-comment-helper.test.ts uses getCommsBaseUri() / apiUrl() instead of hardcoding the base, and asserts notify_audience never leaks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@amix you will really need to have a think on this one. Not having a batch endpoint is going to really screw with the MCP because of just how many calls it sometimes needs to make. I think it could easily result in users getting rate limited. The MCP is why I pushed hard for the batcher to be allowed to be accessed from the SDK. |
|
@amix Was there a discussion around this because I really want to push back hard on this removal. I know you put that Comms no longer exposes the |
|
@scottlovegrove, batching is already gone everywhere. The |
Is this something that needs enabling in the SDK? How does it work if a custom |
|
@scottlovegrove In the browser environment, this should be enabled by default since the browser picks the highest supported HTTP protocol. In Node, it looks like we need to enable it manually. I’ll do a follow-up PR 👍 |
scottlovegrove
left a comment
scottlovegrove
left a comment
There was a problem hiding this comment.
There are a lot of jsdocs been removed as part of this PR and I don't really know why. These docs are what's used to build our documentation site, by removing them, you remove details from the actual SDK docs.
| if (this.baseUrl) { | ||
| const normalizedBaseUrl = this.baseUrl.endsWith('/') ? this.baseUrl : `${this.baseUrl}/` | ||
| return `${normalizedBaseUrl}api/${apiVersion}/` | ||
| return `${normalizedBaseUrl}api/v1/` |
There was a problem hiding this comment.
This should have remained as it was. Why did we need to hardcode it? Why not just update what the default version is?
| import { TEST_API_TOKEN, TEST_COMMENT_ID, TEST_THREAD_ID } from '../testUtils/test-defaults' | ||
|
|
||
| const BASE = 'https://comms.todoist.com/api/v3' | ||
| const BASE = 'https://comms.todoist.com/api/v1' |
There was a problem hiding this comment.
We should have extracted this while we were making changes, then all tests can use it (yes, it bad design on the original code, my bad), instead of having to declare their own identical const.
| * | ||
| * @param args - The arguments for searching. | ||
| * @param args.query - The search query string. Optional when `mentionSelf: true` is set; required otherwise. | ||
| * @param args.workspaceId - The workspace ID to search in. | ||
| * @param args.channelIds - Optional array of channel IDs to filter by. | ||
| * @param args.authorIds - Optional array of author user IDs to filter by. | ||
| * @param args.mentionSelf - Optional flag to filter by mentions of the current user. When true, `query` may be omitted. | ||
| * @param args.dateFrom - Optional start date for filtering (YYYY-MM-DD). | ||
| * @param args.dateTo - Optional end date for filtering (YYYY-MM-DD). | ||
| * @param args.limit - Optional limit on number of results returned. | ||
| * @param args.cursor - Optional cursor for pagination. | ||
| * @param options - Optional configuration. Set `batch: true` to return a descriptor for batch requests. | ||
| * @returns Search results with pagination. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * const results = await api.search.search({ | ||
| * query: 'important meeting', | ||
| import { BaseClient, type ClientConfig } from './base-client' | ||
| * workspaceId: 123 | ||
| * }) | ||
| * ``` | ||
| */ |
There was a problem hiding this comment.
Um, why was all the jsdoc removed with details about the args?
| * @param args - The arguments for searching within a thread. | ||
| * @param args.query - The search query string. | ||
| * @param args.threadId - The thread ID to search in. | ||
| * @param args.limit - Optional limit on number of results returned. | ||
| * @param args.cursor - Optional cursor for pagination. | ||
| * @param options - Optional configuration. Set `batch: true` to return a descriptor for batch requests. | ||
| * @returns Comment IDs that match the search query. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * const results = await api.search.searchThread({ | ||
| * query: 'deadline', | ||
| * threadId: 789 | ||
| * }) | ||
| * ``` |
There was a problem hiding this comment.
Ditto, why was this removed?
| * @param args - The arguments for searching within a conversation. | ||
| * @param args.query - The search query string. | ||
| * @param args.conversationId - The conversation ID to search in. | ||
| * @param args.limit - Optional limit on number of results returned. | ||
| * @param args.cursor - Optional cursor for pagination. | ||
| * @param options - Optional configuration. Set `batch: true` to return a descriptor for batch requests. | ||
| * @returns Message IDs that match the search query. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * const results = await api.search.searchConversation({ | ||
| * query: 'budget', | ||
| * conversationId: 456 | ||
| * }) | ||
| * ``` |
There was a problem hiding this comment.
I guess this applies to all on this file.
| * @param args - The arguments for getting inbox. | ||
| * @param args.workspaceId - The workspace ID. | ||
| * @param args.newerThan - Optional date to get items newer than. | ||
| * @param args.olderThan - Optional date to get items older than. | ||
| * @param args.since - @deprecated Use `newerThan` instead. | ||
| * @param args.until - @deprecated Use `olderThan` instead. | ||
| * @param args.limit - Optional limit on number of items returned. | ||
| * @param args.cursor - Optional cursor for pagination. | ||
| * @param args.archiveFilter - Optional filter: 'active' (default), 'archived', or 'all'. | ||
| * @param options - Optional configuration. Set `batch: true` to return a descriptor for batch requests. | ||
| * @returns Inbox threads. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * const inbox = await api.inbox.getInbox({ | ||
| * workspaceId: 123, | ||
| * newerThan: new Date('2024-01-01') | ||
| * }) | ||
| * | ||
| * // Include archived (done) items alongside active ones | ||
| * const allInbox = await api.inbox.getInbox({ | ||
| * workspaceId: 123, | ||
| * archiveFilter: 'all' | ||
| * }) | ||
| * ``` |
There was a problem hiding this comment.
jsdocs removed, why?
| * Gets unread count for inbox. | ||
| * | ||
| * @param workspaceId - The workspace ID. | ||
| * @param options - Optional configuration. Set `batch: true` to return a descriptor for batch requests. | ||
| * @returns The unread count. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * const count = await api.inbox.getCount(123) | ||
| * console.log(`Unread items: ${count}`) | ||
| * ``` | ||
| */ |
There was a problem hiding this comment.
jsdoc removal question around all that were removed in this file.
| /** | ||
| * Returns a list of workspace user objects for the given workspace id. | ||
| * | ||
| * @param args - The arguments for getting workspace users. | ||
| * @param args.workspaceId - The workspace ID. | ||
| * @param args.archived - Optional flag to filter archived users. | ||
| * @param options - Optional configuration. Set `batch: true` to return a descriptor for batch requests. | ||
| * @returns An array of workspace user objects. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * const users = await api.workspaceUsers.getWorkspaceUsers({ workspaceId: 123 }) | ||
| * users.forEach(u => console.log(u.name, u.userType)) | ||
| * ``` | ||
| */ |
There was a problem hiding this comment.
Jsdoc removal in this file too, why?
| /** | ||
| * Gets all the user's workspaces. | ||
| * | ||
| * @param options - Optional configuration. Set `batch: true` to return a descriptor for batch requests. | ||
| * @returns An array of all workspaces the user belongs to. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * const workspaces = await api.workspaces.getWorkspaces() | ||
| * workspaces.forEach(ws => console.log(ws.name)) | ||
| * ``` | ||
| */ |
There was a problem hiding this comment.
jsdoc removal again.
|
@scottlovegrove the clanker has gone a bit too much overboard with the cleanup 😅 I'll follow up and fix these issuess |
Backs up the "h2 multiplexing replaces /batch" claim from #2 for Node consumers (browsers already negotiate h2 automatically). Without these, Node consumers — including the MCP — were stuck on HTTP/1.1 with near-disabled keep-alive, so every request opened a fresh TCP+TLS connection. Changes: - `EnvHttpProxyAgent({ allowH2: true })` so ALPN can pick h2 when the server offers it (Comms via CloudFront does; fallback to h1.1 is automatic when it doesn't). - Drop the 1ms keepAliveTimeout/keepAliveMaxTimeout hack; use undici defaults so the connection pool actually persists across requests. - Wrap the full dispatcher construction in `suppressExperimentalWarningsSync` so the `allowH2` experimental warning is swallowed alongside the decompress one. - Add `closeDefaultDispatcher()` (exported from the package root) and `CommsApi.close()` so short-lived CLIs / scripts — including ones that only use the OAuth helpers — can drain the pool before exit. Validated direction with expert-letter (Claude + Codex) and Doistbot. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Backs up the "h2 multiplexing replaces /batch" claim from #2 for Node consumers (browsers already negotiate h2 automatically). Without these, Node consumers — including the MCP — were stuck on HTTP/1.1 with near-disabled keep-alive, so every request opened a fresh TCP+TLS connection. Changes: - `EnvHttpProxyAgent({ allowH2: true })` so ALPN can pick h2 when the server offers it (Comms via CloudFront does; fallback to h1.1 is automatic when it doesn't). - Drop the 1ms keepAliveTimeout/keepAliveMaxTimeout hack; use undici defaults so the connection pool actually persists across requests. - Wrap the full dispatcher construction in `suppressExperimentalWarningsSync` so the `allowH2` experimental warning is swallowed alongside the decompress one. - Add `closeDefaultDispatcher()` (exported from the package root) and `CommsApi.close()` so short-lived CLIs / scripts — including ones that only use the OAuth helpers — can drain the pool before exit. Validated direction with expert-letter (Claude + Codex) and Doistbot. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 participants
Context
Comms is a hard fork and no longer exposes a
/batchendpoint, so the SDK should stop advertising it. While I was here, also renamed the API versionv3 → v1(hard fork, no migration).What was changed
src/batch-builder.tsandsrc/types/batch.ts; droppedapi.batch().{ batch: true }/{ batch?: false }/ impl) to a singlePromise<T>signature.users-client.tshelpers (authedGet/authedPost/unauthedPost→get/post).ApiVersion/DEFAULT_API_VERSION/API_VERSIONSmachinery and theversionplumbing onClientConfig/BaseClient/getCommsBaseUri();v1is hardcoded.reactionTarget()returns a discriminated union of SDK-shaped (camelCase) keys; transport snake-cases on the wire.custom-fetch-simple/add-comment-helper/ etc. tests, and the README batch section.Verified:
type-check,oxlint,oxfmt, full Vitest (113/113). Live smoke againstcomms.staging.todoist.comover/api/v1/confirms reads, writes (thread + comment + delete), and the rename ship end-to-end. Cross-checked withexpert-letter(Claude + Codex) and Doistbot.Out of scope
Deferred Doistbot suggestions, tracked as follow-ups:
simple<T>helper intoBaseClientso all clients share one request-and-parse helper.'only_open' | 'all' | 'only_closed'union onChannelsClient.updateFiltersintoFILTER_CLOSED_VALUES(pre-existing pattern from refactor: remove legacy Twist/migration framing #1; worth a sweep across all SDK string unions in one PR).conversations.getOrCreateConversationrejects self-only conversations on the backend).types/entities.ts↔types/enums.ts(CJS build is fine).