generated from coulomb/repo-seed
Add convenience helpers and improve documentation
- Add isInProduction() helper for checking status 3 or 4 - Add getErrorSummary() helper for extracting error messages - Add fetchAllPages() pagination helper for auto-pagination - Add comprehensive JSDoc to ListResponse interface - Create ADR-001 documenting decision not to add listAll() method Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
112
docs/adr/001-no-listall-method.md
Normal file
112
docs/adr/001-no-listall-method.md
Normal file
@@ -0,0 +1,112 @@
|
|||||||
|
# ADR-001: No `listAll()` Method in Documents Client
|
||||||
|
|
||||||
|
**Status:** Accepted
|
||||||
|
**Date:** 2026-01-16
|
||||||
|
**Context:** Response to client feature request (REQ-2 in suggested-improvements.md)
|
||||||
|
|
||||||
|
## Context
|
||||||
|
|
||||||
|
A client integration team requested a `listAll()` method that would:
|
||||||
|
1. Return all documents regardless of status in a single call
|
||||||
|
2. Support filtering by status codes
|
||||||
|
3. Optionally auto-paginate to fetch all pages
|
||||||
|
|
||||||
|
The Binect REST API exposes two distinct endpoints for listing documents:
|
||||||
|
- `GET /documents` - Returns shippable documents (status 2)
|
||||||
|
- `GET /documents/errors` - Returns erroneous documents (status 7)
|
||||||
|
|
||||||
|
There is no single API endpoint that returns documents across all statuses.
|
||||||
|
|
||||||
|
## Decision
|
||||||
|
|
||||||
|
We will **not** implement a `listAll()` method on the `DocumentsClient` class.
|
||||||
|
|
||||||
|
## Rationale
|
||||||
|
|
||||||
|
### 1. Violates Core Design Principle: 1:1 Semantic Mapping
|
||||||
|
|
||||||
|
The SDK's architecture mandates a "1:1 semantic mapping to REST endpoints" (CLAUDE.md). A `listAll()` method would need to:
|
||||||
|
- Call multiple API endpoints (`/documents` and `/documents/errors`)
|
||||||
|
- Merge and deduplicate results
|
||||||
|
- Present a unified interface that doesn't exist in the underlying API
|
||||||
|
|
||||||
|
This creates abstraction that obscures actual API behavior.
|
||||||
|
|
||||||
|
### 2. Violates Transparency Principle
|
||||||
|
|
||||||
|
The design constraint "Transparency over abstraction: Developers must reason about actual API calls" means developers should understand exactly which API calls are being made. A `listAll()` method hides the fact that multiple calls occur, making it harder to:
|
||||||
|
- Debug network issues
|
||||||
|
- Understand rate limiting implications
|
||||||
|
- Reason about data freshness (documents may change between calls)
|
||||||
|
|
||||||
|
### 3. Incomplete Coverage Cannot Be Fixed
|
||||||
|
|
||||||
|
Even if we implemented `listAll()`, we cannot retrieve documents in all statuses:
|
||||||
|
- Status 1 (IN_PREPARATION) - No list endpoint
|
||||||
|
- Status 3 (PRODUCTION_QUEUE) - No list endpoint
|
||||||
|
- Status 4 (PRINTING) - No list endpoint
|
||||||
|
- Status 5 (SENT) - No list endpoint
|
||||||
|
- Status 6 (CANCELED) - No list endpoint
|
||||||
|
|
||||||
|
A method named `listAll()` that doesn't actually list all documents would be misleading.
|
||||||
|
|
||||||
|
### 4. Convenience Layer Alternative Exists
|
||||||
|
|
||||||
|
Developers who need to combine results can do so explicitly:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
import { fetchAllPages } from '@binect/js';
|
||||||
|
|
||||||
|
// Explicit composition - developer knows exactly what's happening
|
||||||
|
const [shippable, erroneous] = await Promise.all([
|
||||||
|
fetchAllPages((limit, offset) => client.documents.list({ limit, offset })),
|
||||||
|
fetchAllPages((limit, offset) => client.documents.listErrors({ limit, offset })),
|
||||||
|
]);
|
||||||
|
|
||||||
|
const allAvailable = [...shippable, ...erroneous];
|
||||||
|
```
|
||||||
|
|
||||||
|
This approach:
|
||||||
|
- Makes the multiple API calls explicit
|
||||||
|
- Allows parallel execution for better performance
|
||||||
|
- Gives developers control over error handling per endpoint
|
||||||
|
- Doesn't pretend to offer functionality the API doesn't support
|
||||||
|
|
||||||
|
## Consequences
|
||||||
|
|
||||||
|
### Positive
|
||||||
|
- SDK remains a transparent wrapper over the API
|
||||||
|
- No false promises about "all documents" when API doesn't support it
|
||||||
|
- Developers understand the actual API limitations
|
||||||
|
- Easier to maintain as SDK behavior matches API exactly
|
||||||
|
|
||||||
|
### Negative
|
||||||
|
- Developers must write slightly more code to combine results
|
||||||
|
- May be perceived as less convenient than competing SDKs
|
||||||
|
|
||||||
|
## Alternatives Considered
|
||||||
|
|
||||||
|
### Alternative 1: Add `listAll()` to Convenience Layer (helpers.ts)
|
||||||
|
|
||||||
|
We considered adding a helper function rather than a method:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
export async function listAllDocuments(client: BinectClient): Promise<Document[]>
|
||||||
|
```
|
||||||
|
|
||||||
|
**Rejected because:** Even as a helper, the name "listAll" is misleading since it cannot list documents in statuses 1, 3, 4, 5, or 6. A more honest name would be `listShippableAndErroneous()`, which provides little value over explicit composition.
|
||||||
|
|
||||||
|
### Alternative 2: Add Status Filter to Existing Methods
|
||||||
|
|
||||||
|
We considered adding a status filter parameter to `list()`:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
await client.documents.list({ status: [2, 7] });
|
||||||
|
```
|
||||||
|
|
||||||
|
**Rejected because:** The API doesn't support this. We would be inventing client-side filtering semantics that don't map to any API capability.
|
||||||
|
|
||||||
|
## References
|
||||||
|
|
||||||
|
- CLAUDE.md - Design Constraints section
|
||||||
|
- suggested-improvements.md - REQ-2: Add Method to List All Documents
|
||||||
101
src/helpers.ts
101
src/helpers.ts
@@ -5,7 +5,7 @@
|
|||||||
* They provide convenient predicates and utilities for common operations.
|
* They provide convenient predicates and utilities for common operations.
|
||||||
*/
|
*/
|
||||||
|
|
||||||
import { DocumentStatus, type Document, type Sending, type ValidationMessage } from './types.js';
|
import { DocumentStatus, type Document, type ListResponse, type Sending, type ValidationMessage } from './types.js';
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Helper to get status code from document or sending
|
* Helper to get status code from document or sending
|
||||||
@@ -71,6 +71,18 @@ export function isCanceled(doc: Document | Sending): boolean {
|
|||||||
return getStatusCode(doc) === DocumentStatus.CANCELED;
|
return getStatusCode(doc) === DocumentStatus.CANCELED;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Check if a document is in production (status 3 or 4).
|
||||||
|
* This includes documents in the production queue and those currently printing.
|
||||||
|
*/
|
||||||
|
export function isInProduction(doc: Document | Sending): boolean {
|
||||||
|
const status = getStatusCode(doc);
|
||||||
|
return (
|
||||||
|
status === DocumentStatus.PRODUCTION_QUEUE ||
|
||||||
|
status === DocumentStatus.PRINTING
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Check if a document is in a terminal state (sent, canceled, or erroneous).
|
* Check if a document is in a terminal state (sent, canceled, or erroneous).
|
||||||
*/
|
*/
|
||||||
@@ -140,6 +152,30 @@ export function hasWarnings(doc: Document): boolean {
|
|||||||
return getWarnings(doc).length > 0;
|
return getWarnings(doc).length > 0;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Get a summary of all error messages from a document.
|
||||||
|
* Returns undefined if the document has no errors.
|
||||||
|
*
|
||||||
|
* @param doc - The document to extract error messages from
|
||||||
|
* @param separator - Separator between messages (default: '; ')
|
||||||
|
* @returns Concatenated error messages or undefined if no errors
|
||||||
|
*
|
||||||
|
* @example
|
||||||
|
* ```typescript
|
||||||
|
* const doc = await client.documents.get(id);
|
||||||
|
* if (isErroneous(doc)) {
|
||||||
|
* console.error('Upload failed:', getErrorSummary(doc));
|
||||||
|
* }
|
||||||
|
* ```
|
||||||
|
*/
|
||||||
|
export function getErrorSummary(doc: Document, separator: string = '; '): string | undefined {
|
||||||
|
const errors = getErrors(doc);
|
||||||
|
if (errors.length === 0) {
|
||||||
|
return undefined;
|
||||||
|
}
|
||||||
|
return errors.map((e) => e.message).join(separator);
|
||||||
|
}
|
||||||
|
|
||||||
// ============================================================================
|
// ============================================================================
|
||||||
// Status Description
|
// Status Description
|
||||||
// ============================================================================
|
// ============================================================================
|
||||||
@@ -272,3 +308,66 @@ export async function waitForShippable(
|
|||||||
options
|
options
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ============================================================================
|
||||||
|
// Pagination Utilities
|
||||||
|
// ============================================================================
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Options for fetchAllPages helper.
|
||||||
|
*/
|
||||||
|
export interface FetchAllPagesOptions {
|
||||||
|
/** Number of items per page (default: 100) */
|
||||||
|
pageSize?: number;
|
||||||
|
/** Abort signal for cancellation */
|
||||||
|
signal?: AbortSignal;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Fetch all pages from a paginated list endpoint.
|
||||||
|
* This is an opt-in convenience helper for retrieving complete result sets.
|
||||||
|
*
|
||||||
|
* @param fetchPage - Function that fetches a single page given limit and offset
|
||||||
|
* @param options - Pagination options
|
||||||
|
* @returns Array of all items from all pages
|
||||||
|
*
|
||||||
|
* @example
|
||||||
|
* ```typescript
|
||||||
|
* // Fetch all shippable documents
|
||||||
|
* const allDocs = await fetchAllPages(
|
||||||
|
* (limit, offset) => client.documents.list({ limit, offset })
|
||||||
|
* );
|
||||||
|
*
|
||||||
|
* // Fetch all erroneous documents with custom page size
|
||||||
|
* const allErrors = await fetchAllPages(
|
||||||
|
* (limit, offset) => client.documents.listErrors({ limit, offset }),
|
||||||
|
* { pageSize: 50 }
|
||||||
|
* );
|
||||||
|
* ```
|
||||||
|
*/
|
||||||
|
export async function fetchAllPages<T>(
|
||||||
|
fetchPage: (limit: number, offset: number) => Promise<ListResponse<T>>,
|
||||||
|
options: FetchAllPagesOptions = {}
|
||||||
|
): Promise<T[]> {
|
||||||
|
const pageSize = options.pageSize ?? 100;
|
||||||
|
const allItems: T[] = [];
|
||||||
|
let offset = 0;
|
||||||
|
|
||||||
|
while (true) {
|
||||||
|
if (options.signal?.aborted) {
|
||||||
|
throw new Error('Fetch aborted');
|
||||||
|
}
|
||||||
|
|
||||||
|
const response = await fetchPage(pageSize, offset);
|
||||||
|
allItems.push(...response.items);
|
||||||
|
|
||||||
|
// Check if we've fetched all items
|
||||||
|
if (response.items.length < pageSize || allItems.length >= response.total) {
|
||||||
|
break;
|
||||||
|
}
|
||||||
|
|
||||||
|
offset += pageSize;
|
||||||
|
}
|
||||||
|
|
||||||
|
return allItems;
|
||||||
|
}
|
||||||
|
|||||||
@@ -77,12 +77,14 @@ export {
|
|||||||
isCanceled,
|
isCanceled,
|
||||||
isTerminal,
|
isTerminal,
|
||||||
isCancelable,
|
isCancelable,
|
||||||
|
isInProduction,
|
||||||
// Validation helpers
|
// Validation helpers
|
||||||
getErrors,
|
getErrors,
|
||||||
getWarnings,
|
getWarnings,
|
||||||
getInfoMessages,
|
getInfoMessages,
|
||||||
hasErrors,
|
hasErrors,
|
||||||
hasWarnings,
|
hasWarnings,
|
||||||
|
getErrorSummary,
|
||||||
// Status description
|
// Status description
|
||||||
getStatusDescription,
|
getStatusDescription,
|
||||||
// Base64 utilities
|
// Base64 utilities
|
||||||
@@ -92,4 +94,7 @@ export {
|
|||||||
pollUntil,
|
pollUntil,
|
||||||
waitForShippable,
|
waitForShippable,
|
||||||
type PollOptions,
|
type PollOptions,
|
||||||
|
// Pagination utilities
|
||||||
|
fetchAllPages,
|
||||||
|
type FetchAllPagesOptions,
|
||||||
} from './helpers.js';
|
} from './helpers.js';
|
||||||
|
|||||||
25
src/types.ts
25
src/types.ts
@@ -313,12 +313,35 @@ export interface Document {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* List response wrapper
|
* Response structure for paginated list operations.
|
||||||
|
*
|
||||||
|
* @remarks
|
||||||
|
* The actual data is in the `items` array, not `data` or `results`.
|
||||||
|
*
|
||||||
|
* @example
|
||||||
|
* ```typescript
|
||||||
|
* const response = await client.documents.list();
|
||||||
|
*
|
||||||
|
* // Access the documents array via `items`
|
||||||
|
* for (const doc of response.items) {
|
||||||
|
* console.log(doc.filename, doc.status.text);
|
||||||
|
* }
|
||||||
|
*
|
||||||
|
* // Pagination information
|
||||||
|
* console.log(`Showing ${response.items.length} of ${response.total} documents`);
|
||||||
|
*
|
||||||
|
* // Check if more pages exist
|
||||||
|
* const hasMore = response.offset + response.items.length < response.total;
|
||||||
|
* ```
|
||||||
*/
|
*/
|
||||||
export interface ListResponse<T> {
|
export interface ListResponse<T> {
|
||||||
|
/** Array of items returned by the query */
|
||||||
items: T[];
|
items: T[];
|
||||||
|
/** Total number of items matching the query (across all pages) */
|
||||||
total: number;
|
total: number;
|
||||||
|
/** Maximum number of items per page */
|
||||||
limit: number;
|
limit: number;
|
||||||
|
/** Number of items skipped (for pagination) */
|
||||||
offset: number;
|
offset: number;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user