Skip to content

Commit ea15653

Browse files
committed
chore: add migration tracking scripts and docs
1 parent 37078d3 commit ea15653

4 files changed

Lines changed: 969 additions & 8 deletions

File tree

docs/api-endpoint-migration.md

Lines changed: 220 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -358,22 +358,29 @@ integrations: {
358358

359359
### Handling nullable types
360360

361-
When a field can be `null`, combine `nullable: true` with `$ref`:
361+
Ajv does **not** allow `nullable: true` without `type`. Since `$ref` schemas don't have a `type` at the reference site, you cannot use `{ nullable: true, $ref: '...' }`. Instead, use `oneOf` with `{ type: 'null' }`:
362362

363363
```typescript
364-
// Nullable $ref
365-
report: { nullable: true, $ref: '#/components/schemas/IModerationReport' },
364+
// Nullable $ref — use oneOf with null
365+
report: {
366+
oneOf: [
367+
{ $ref: '#/components/schemas/IModerationReport' },
368+
{ type: 'null' },
369+
],
370+
},
366371

367-
// Nullable union
372+
// Nullable union — add null as another oneOf branch
368373
integration: {
369-
nullable: true,
370374
oneOf: [
371375
{ $ref: '#/components/schemas/IIncomingIntegration' },
372376
{ $ref: '#/components/schemas/IOutgoingIntegration' },
377+
{ type: 'null' },
373378
],
374379
},
375380
```
376381

382+
> **Note**: `nullable: true` works fine with `type`, e.g., `{ type: 'string', nullable: true }`. The restriction only applies when combining `nullable` with `$ref` (no `type` present).
383+
377384
### Handling intersection types with `allOf`
378385

379386
When a response intersects a type with additional properties (e.g., `VideoConferenceInstructions & { providerName: string }`), use `allOf`:
@@ -440,6 +447,44 @@ This happens because `oneOf` requires **exactly one** match, but the value is bo
440447

441448
**Long-term fix**: Revise the core-typings to narrow `ts` to `string` (which is what MongoDB aggregation pipelines and `JSON.stringify` actually return), or adjust the AJV/typia schema generation to handle `Date | string` unions correctly (e.g., using `anyOf` instead of `oneOf`, or collapsing `Date` into `string`).
442449

450+
### Known Pitfall: Mapped utility types (`Nullable<>`, `Partial<>`, etc.)
451+
452+
Typia does **not** resolve custom mapped types when generating JSON schemas. For example, the following pattern:
453+
454+
```typescript
455+
type Nullable<T, K extends keyof T> = { [P in K]: T[P] | null } & Omit<T, K>;
456+
457+
export interface IVideoConferenceUser
458+
extends Nullable<Pick<Required<IUser>, '_id' | 'username' | 'name' | 'avatarETag'>, 'avatarETag'> {
459+
ts: Date;
460+
}
461+
```
462+
463+
Produces `avatarETag: { type: 'string' }` **without** `nullable: true`, even though the resolved type is `string | null`. This causes response validation to reject `null` values when `coerceTypes` is `false`.
464+
465+
**Fix**: Declare nullable fields explicitly instead of relying on mapped types:
466+
467+
```typescript
468+
// BEFORE — typia loses the | null
469+
extends Nullable<Pick<Required<IUser>, '_id' | 'username' | 'name' | 'avatarETag'>, 'avatarETag'>
470+
471+
// AFTER — typia correctly generates nullable: true
472+
extends Pick<Required<IUser>, '_id' | 'username' | 'name'> {
473+
avatarETag: string | null;
474+
}
475+
```
476+
477+
After changing the type, rebuild core-typings (`yarn workspace @rocket.chat/core-typings run build`) to regenerate the schema.
478+
479+
**How to detect**: If a `$ref` schema rejects `null` values at runtime but the TypeScript type allows `null`, check if the type uses a mapped utility type. Inspect the generated schema:
480+
481+
```bash
482+
node -e "const { schemas } = require('./packages/core-typings/dist/Ajv.js'); \
483+
console.log(JSON.stringify(schemas.components.schemas.YourType, null, 2))"
484+
```
485+
486+
Look for fields that should have `nullable: true` but don't.
487+
443488
### Adding a new type to typia
444489

445490
If you need a `$ref` for a type that is not yet registered:
@@ -524,6 +569,36 @@ Source: `apps/meteor/app/api/server/v1/invites.ts`
524569
4. **Augment `Endpoints`**: Use `declare module '@rocket.chat/rest-typings'` to merge the extracted types into the global `Endpoints` interface. This is what makes `useEndpoint('listInvites')` and similar SDK calls type-safe.
525570
5. **Import `ExtractRoutesFromAPI`** from `'../ApiClass'` (relative to the endpoint file in `v1/`).
526571

572+
### Keep types in `rest-typings` (do NOT remove them)
573+
574+
The `declare module` augmentation via `ExtractRoutesFromAPI` only works within the `apps/meteor` compilation unit. External packages (`ddp-client`, `rest-client`, etc.) compile independently and **do not see** the augmented types — they only see the types exported from `@rocket.chat/rest-typings`.
575+
576+
**When migrating an endpoint, keep its type definition in `rest-typings` unchanged.** The augmentation adds response schema types on top of the existing definition. Removing the type from `rest-typings` will break external package consumers.
577+
578+
This duplication is temporary — see `docs/api-definitions-package-plan.md` for the planned consolidation.
579+
580+
### Use `as const` on options variables
581+
582+
When endpoint options are stored in a separate variable (required for sharing between action factories), add `as const` so that `authRequired: true` is inferred as the literal `true`, not `boolean`. This matters because `TypedThis` uses a conditional type:
583+
584+
```typescript
585+
userId: TOptions['authRequired'] extends true ? string : string | undefined;
586+
```
587+
588+
Without `as const`, `authRequired` is `boolean`, and `userId` becomes `string | undefined` — forcing unnecessary guards in the action body.
589+
590+
```typescript
591+
// Correct — userId is string, bodyParams is typed
592+
const myEndpointProps = {
593+
authRequired: true,
594+
body: isMyProps,
595+
response: { ... },
596+
} as const;
597+
598+
// Inline options don't need as const — TypeScript infers literals from the generic context
599+
API.v1.post('myEndpoint', { authRequired: true, body: isMyProps, response: { ... } }, async function action() { ... });
600+
```
601+
527602
### What augmentation enables
528603

529604
Once the `Endpoints` interface is augmented, the entire stack benefits:
@@ -598,15 +673,59 @@ When migrating an endpoint, search for its tests and update:
598673

599674
## Tracking Migration Progress
600675

676+
Two scripts help track migration progress and identify type-safety issues.
677+
678+
### `scripts/list-unmigrated-api-endpoints.mjs`
679+
680+
Lists all endpoints that still use the legacy `addRoute` pattern and need migration.
681+
601682
```bash
602-
# Summary by file
683+
# Human-readable report grouped by file
603684
node scripts/list-unmigrated-api-endpoints.mjs
604685

605-
# Full list with line numbers (JSON)
686+
# Machine-readable JSON with file paths and line numbers
606687
node scripts/list-unmigrated-api-endpoints.mjs --json
607688
```
608689

609-
The script scans for `API.v1.addRoute` and `API.default.addRoute` calls in `apps/meteor/app/api/`.
690+
The script scans `apps/meteor/app/api/` for `API.v1.addRoute(...)` and `API.default.addRoute(...)` calls, extracting the route path, HTTP methods, file, and line number. Endpoints using this pattern lack compile-time type checking on request params and response shapes.
691+
692+
### `scripts/analyze-weak-types.mjs`
693+
694+
Analyzes `packages/rest-typings/` for "weak" types — generic types that provide little or no type safety in endpoint definitions.
695+
696+
```bash
697+
# Full report grouped by endpoint
698+
node scripts/analyze-weak-types.mjs
699+
700+
# JSON output for tooling/CI
701+
node scripts/analyze-weak-types.mjs --json
702+
703+
# Only check AJV schema definitions (type: 'object' with no properties, etc.)
704+
node scripts/analyze-weak-types.mjs --schema-only
705+
706+
# Only check TypeScript type definitions (any, Record<string, any>, etc.)
707+
node scripts/analyze-weak-types.mjs --ts-only
708+
```
709+
710+
Weak types detected:
711+
712+
| Pattern | Level | Risk |
713+
|---|---|---|
714+
| `any`, `unknown` | TypeScript | No type checking at all |
715+
| `object` (bare) | TypeScript | Accepts any non-primitive |
716+
| `Record<string, any>`, `Record<string, unknown>` | TypeScript | Untyped key-value bag |
717+
| `any[]`, `Array<any>` | TypeScript | Untyped array |
718+
| `Partial<IMessage>`, `Partial<IRoom>`, `Partial<IUser>` | TypeScript | Overly broad — accepts any subset of a large interface |
719+
| `{ type: 'object' }` without `properties` | AJV Schema | No runtime validation of object shape |
720+
| `{ type: 'array' }` without `items` | AJV Schema | No runtime validation of array elements |
721+
722+
### Recommended workflow
723+
724+
1. **Pick a domain** to migrate (e.g., channels, users, teams).
725+
2. **Run `list-unmigrated-api-endpoints.mjs`** to see which endpoints still use `addRoute`.
726+
3. **Run `analyze-weak-types.mjs`** to check if the existing `rest-typings` for that domain have weak types.
727+
4. **Migrate the endpoint**: move from `addRoute` to the typed pattern, and strengthen any weak types in `rest-typings` at the same time.
728+
5. **Re-run both scripts** to verify the endpoint no longer appears in either report.
610729

611730
## Reference Files
612731

@@ -625,3 +744,96 @@ The script scans for `API.v1.addRoute` and `API.default.addRoute` calls in `apps
625744
| Request validators (examples) | `packages/rest-typings/src/v1/moderation/` |
626745
| Router implementation | `packages/http-router/src/Router.ts` |
627746
| Unmigrated endpoints script | `scripts/list-unmigrated-api-endpoints.mjs` |
747+
| Weak types analysis script | `scripts/analyze-weak-types.mjs` |
748+
749+
## Post-Migration Cleanup Checklist
750+
751+
After migrating endpoints, run through these improvements to maximize quality. These are not blocking but should be addressed before the migration is considered complete.
752+
753+
### 1. Extract shared schemas
754+
755+
Avoid duplicating the same schema inline across endpoints. Common patterns to extract:
756+
757+
```typescript
758+
// Void success — reuse across all endpoints returning only { success: true }
759+
const voidSuccessResponse = ajv.compile<void>({
760+
type: 'object',
761+
properties: { success: { type: 'boolean', enum: [true] } },
762+
required: ['success'],
763+
additionalProperties: false,
764+
});
765+
766+
// Paginated response — reuse for list endpoints
767+
const paginatedUsersResponse = ajv.compile<{ users: IUser[]; count: number; offset: number; total: number }>({
768+
type: 'object',
769+
properties: {
770+
users: { type: 'array' },
771+
count: { type: 'number' },
772+
offset: { type: 'number' },
773+
total: { type: 'number' },
774+
success: { type: 'boolean', enum: [true] },
775+
},
776+
required: ['users', 'count', 'offset', 'total', 'success'],
777+
additionalProperties: false,
778+
});
779+
780+
// User identifier body — reuse for endpoints accepting userId/username/user
781+
const userIdentifierBodySchema = ajv.compile<{ userId?: string; username?: string; user?: string }>({
782+
type: 'object',
783+
properties: {
784+
userId: { type: 'string' },
785+
username: { type: 'string' },
786+
user: { type: 'string' },
787+
},
788+
additionalProperties: false,
789+
});
790+
```
791+
792+
**Important:** Declare shared `const` schemas **before** their first usage to avoid temporal dead zone (TDZ) errors. JavaScript `const` is not hoisted.
793+
794+
### 2. Strengthen relaxed object schemas
795+
796+
After initial migration, review schemas using `{ type: 'object' }` without `properties`. These pass any object shape through without validation.
797+
798+
**Acceptable cases** (document with a comment):
799+
- Dynamic/schemaless objects (e.g., user preferences, custom fields)
800+
- Complex types not yet in typia (e.g., `IExportOperation`)
801+
802+
**Should be fixed:**
803+
- Types that have a `$ref` available (e.g., `IUser``{ $ref: '#/components/schemas/IUser' }`)
804+
- Array items without schema (e.g., `{ type: 'array' }``{ type: 'array', items: { $ref: '...' } }`)
805+
806+
Run `node scripts/analyze-weak-types.mjs --schema-only` to find all instances.
807+
808+
### 3. Add `authRequired: false` explicitly
809+
810+
Endpoints that are intentionally public should declare `authRequired: false` explicitly rather than relying on the default. This makes the intent clear and self-documenting.
811+
812+
### 4. Add OpenAPI tags
813+
814+
Endpoints without `tags` in their options are excluded from the generated OpenAPI spec. Add appropriate tags:
815+
816+
```typescript
817+
{
818+
authRequired: true,
819+
tags: ['Users'],
820+
// ...
821+
}
822+
```
823+
824+
### 5. Create a changeset
825+
826+
If the migration changes error types (e.g., `'invalid-params'``'error-invalid-params'`), this is a breaking change for API consumers matching on `errorType`. Create a changeset:
827+
828+
```bash
829+
yarn changeset
830+
```
831+
832+
Use `minor` bump for the affected packages and document the error type change.
833+
834+
### 6. Add missing test coverage
835+
836+
For each migrated endpoint, verify:
837+
- **Validator tests** exist for the body/query schema (valid, invalid, extra properties)
838+
- **E2E tests** cover success (200), unauthorized (401), invalid params (400), and forbidden (403) cases
839+
- **Error type assertions** use `'error-invalid-params'` (not the old `'invalid-params'`)

0 commit comments

Comments
 (0)