diff --git a/.agent/rules/porting.mdc b/.agent/rules/porting.mdc index 0f5caf5f..183ab4b9 100644 --- a/.agent/rules/porting.mdc +++ b/.agent/rules/porting.mdc @@ -52,6 +52,7 @@ TypeScript: | Go | TypeScript | | ---------------------------- | ----------------------------------- | +| `databricks/api/` | `packages/databricks/src/api/` | | `databricks/apierr/` | `packages/databricks/src/apierror/` | | `databricks/apierr/codes/` | `packages/databricks/src/apierror/codes/` | @@ -138,6 +139,22 @@ tc.want.httpErr" or "equivalent of Go's errors.As." // Input HTTP fields are always preserved. ``` +## Value Types vs Reference Types + +Go structs are value types — passing a struct to a function copies it. TypeScript +objects are reference types — passing an object shares it. When Go code relies on +value-copy semantics (e.g. a retrier getting its own copy of a backoff policy), +the TypeScript port must account for this. Common approaches: + +1. **Accept options, create internally** — the function takes a plain options + object and creates its own instance. This is the most ergonomic. +2. **Clone internally** — the function accepts an instance but clones it. +3. **Document the constraint** — accept an instance, document that each caller + must provide a fresh one. Simple but error-prone. + +Do not silently share mutable state between callers. Explain the chosen approach +to the user as a deviation. + ## Deviations from Go When the TypeScript implementation must differ from Go (e.g. different function diff --git a/.agent/rules/testing.mdc b/.agent/rules/testing.mdc index 3ee7ccce..b3bdd1c0 100644 --- a/.agent/rules/testing.mdc +++ b/.agent/rules/testing.mdc @@ -129,6 +129,30 @@ const testCases: {desc: string; want?: {code: Code; message: string}}[] = [ ]; ``` +## Mocking Module-Level Functions + +To mock a module-level function in ESM, the function must be a method on an +exported object — not a standalone exported function. Standalone functions are +captured as local closure references and `vi.spyOn` cannot intercept them. + +```typescript +// In the source file: +export const rand = { + int(n: number): number { + return Math.floor(Math.random() * n); + }, +}; + +// In the test file: +import {rand} from '../../src/api/retrier'; + +vi.spyOn(rand, 'int').mockImplementation(n => n - 1); +// ... run test ... +vi.restoreAllMocks(); +``` + +Always call `vi.restoreAllMocks()` after mocking to avoid test pollution. + ## Independence Each test must be independent. Do not rely on execution order. Clean up side diff --git a/.agent/rules/typescript.mdc b/.agent/rules/typescript.mdc index 732b2261..889d6bc2 100644 --- a/.agent/rules/typescript.mdc +++ b/.agent/rules/typescript.mdc @@ -443,11 +443,88 @@ Avoid private static methods; prefer module-level functions instead. Never use Always include parentheses in constructor calls: `new Foo()`, not `new Foo`. -### 7.7 API Surface Minimization +### 7.7 Internal Methods and `@private` JSDoc + +When a method or constructor must be accessible to internal code and tests but +should not be used by consumers, mark it with `@private` JSDoc. Do not export +the options interface from `index.ts`. + +```typescript +/** + * Do not use this constructor directly. Use {@link MyClass.create} instead. + * This constructor is only meant for internal and testing use. + * + * @private + */ +constructor(options: MyClassOptions) { ... } +``` + +**When users need to instantiate a class directly** (no factory function), the +constructor must be public and the options interface **must** be exported from +`index.ts`. A public constructor with an unexported options type gives consumers +no autocomplete and no documentation — that is a broken experience. Only use +`@private` JSDoc when there is a dedicated factory function (e.g. +`fromHttpError`). + +### 7.8 API Surface Minimization Default to the smallest public API surface. Only export from `index.ts` what consumers need. Internal types (options interfaces, schemas, helper functions) -must not be re-exported. +must not be re-exported. Use `@private` JSDoc for methods that are technically +accessible but not part of the public contract. + +### 7.9 Consumer Experience + +When creating any new public export, always consider: + +- Can the consumer see the types? (Are options interfaces exported?) +- Do they get autocomplete and JSDoc in their IDE? +- Is it clear how to use the API without reading the source? + +Provide consumer usage examples to the user when proposing new public APIs. + +### 7.10 Testing Hooks + +Testing hooks (e.g. a custom random number generator for deterministic tests) +must never appear in constructor options or public interfaces. Instead, use a +module-level object that tests can mock with `vi.spyOn`. + +Standalone module-level functions cannot be mocked with `vi.spyOn` in ESM. +In ESM, when a module defines and calls its own function, the internal call is +bound to the function's local variable at module evaluation time — not to the +module's export binding. `vi.spyOn(module, 'fn')` replaces the export binding, +but the class's method still holds a closure over the original function. The +spy intercepts what *importers* see, but not what the module's *own code* calls. + +Wrapping the function in an object (`rand.int()`) fixes this because the call +goes through a property lookup on the object at runtime. `vi.spyOn(rand, 'int')` +replaces the property on the object itself, so any code that calls `rand.int()` +— including internal code in the same module — sees the mock. + +```typescript +// Good — testable via vi.spyOn(rand, 'int'). +export const rand = { + int(n: number): number { + return Math.floor(Math.random() * n); + }, +}; + +class BackoffPolicy { + delay(): number { + return rand.int(this.current + 1); + } +} + +// Bad — randomInt in constructor options pollutes the public API. +interface BackoffPolicyOptions { + randomInt?: (n: number) => number; // Do not do this. +} + +// Bad — standalone function cannot be mocked in ESM. +export function randomInt(n: number): number { ... } +``` + +Do not export testing objects from `index.ts` — they are internal. ---