diff --git a/.changeset/10078-record-id-string-survivors.md b/.changeset/10078-record-id-string-survivors.md new file mode 100644 index 0000000000..316d0812af --- /dev/null +++ b/.changeset/10078-record-id-string-survivors.md @@ -0,0 +1,59 @@ +--- +'@object-ui/types': minor +'@object-ui/core': minor +'@object-ui/data-objectstack': minor +--- + +**BREAKING** — three more record ids that the objectui#9511 ruling's enumeration left out are +strings now (objectui#10078). `CommentSearchResult.recordId`, `RecordSubscription.recordId` +and `DataSourceMutationEvent.id` narrow **FROM** `string | number` **TO** `string`. ⚠️ Other +record ids in `@object-ui/types` still admit a number and are not touched here, among them +`DetailViewSchema.recordNavigation` (`recordIds`, `onNavigate`), `FeedItem.sourceId`, and the +`onNavigate` slots on `ObjectGridSchema`, `ObjectViewSchema` and `ListViewRuntimeProps`. + +```ts +// before — compiled +const bell: RecordSubscription = { recordId: 42, subscribed: true }; +// after — refused by the compiler; write the id the protocol carries +const bell: RecordSubscription = { recordId: '42', subscribed: true }; +``` + +**Why.** objectui#9511 (director batch #195 item 1, letter A) ruled that a record id is a +string wherever **metadata** names one, and extended that, as 「the same principle」, to +`CommentEntry.recordId` and `MentionNotification.recordId`. It did not name these three, which +are runtime and API shapes rather than metadata. Carrying the rule to them is the `domain:spec` +seat's inheritance decision, recorded in the claim on objectui#10078: they hold the same id and +were left with the old spelling. `CommentSearchResult.recordId` is copied straight from +`CommentEntry.recordId`, `RecordSubscription.recordId` names the record a notification bell is +for, and `DataSourceMutationEvent.id` is the id the `update` / `delete` doors already take as a +string. + +**What changes at runtime.** `ValueDataSource` and `ObjectStackAdapter` now emit +`onMutation` events whose `id` is always a string. Both methods still accept a numeric id +through their own wider parameter, and each converts it at its own boundary before emitting — +the one place the rule says the conversion lives. An in-memory `value` source whose items +carry numeric keys therefore announces `'42'`, not `42`. A subscriber that compared +`event.id` against a number must compare against the string. The two in-repo subscribers +that read `event.id` already pass it through `String(...)`, so they see no difference. + +**Packages that narrow without a source change.** `@object-ui/collaboration` (`useCommentSearch` +returns `CommentSearchResult[]`) and `@object-ui/plugin-detail` (`SubscriptionToggleProps` +carries a `RecordSubscription`) publish the narrower type through their own `.d.ts`. + +**What a host has to do.** Code that builds a `RecordSubscription`, reads +`CommentSearchResult.recordId`, or implements `DataSource.onMutation` with numeric keys +converts once, where the number enters (its own adapter or data boundary), and passes the +string from there on. ⛔ Nothing converts silently on the reading side. + +**Disposition: ADR-0087 D7 — a published runtime TypeScript interface with no metadata +surface is carried by the compiler and needs no ledger entry; ⛔ no conversion entry, ⛔ not +a tombstone, ⛔ not an npm `major`.** None of the three has a zod mirror, none is authored into JSON, and no schema +references them, so there is no parse-time door and nothing for a conversion table to +rewrite: the compiler error at the numeric site is the delivery channel. The level is `minor` +because objectui marks its own breaking changes `minor` with this banner (AGENTS.md, version +alignment); `major` is refused outright because every publishable package sits in one `fixed` +group pinned to the `@objectstack` major (`scripts/check-changeset-no-major.mjs`). + +**Pin.** `record-id-string-survivors-10078.test.ts` in `@object-ui/types` asserts all three +members read `string` (not `any`), refuses a numeric literal at each, and keeps a +source-text census of the three declarations with controls that prove the census can fire. diff --git a/content/docs/guide/data-source.md b/content/docs/guide/data-source.md index 6f06c2a06b..bf771e6ef2 100644 --- a/content/docs/guide/data-source.md +++ b/content/docs/guide/data-source.md @@ -15,17 +15,17 @@ import type { BatchTransactionOperation, QueryParams, QueryResult } from '@objec export interface DataSource { find(resource: string, params?: QueryParams): Promise>; - findOne(resource: string, id: string | number, params?: QueryParams): Promise; + findOne(resource: string, id: string, params?: QueryParams): Promise; create(resource: string, data: Partial): Promise; update( resource: string, - id: string | number, + id: string, data: Partial, opts?: { ifMatch?: string }, ): Promise; delete( resource: string, - id: string | number, + id: string, opts?: { ifMatch?: string }, ): Promise; diff --git a/content/docs/plugins/plugin-detail.mdx b/content/docs/plugins/plugin-detail.mdx index 9ac56bb668..60c74dd8bf 100644 --- a/content/docs/plugins/plugin-detail.mdx +++ b/content/docs/plugins/plugin-detail.mdx @@ -398,7 +398,7 @@ decision as a defect is how a warning channel gets ignored: ```plaintext { - recordId: string | number, + recordId: string, subscribed: boolean, channels?: Array<'in_app' | 'email' | 'push'> } diff --git a/packages/core/src/adapters/ValueDataSource.ts b/packages/core/src/adapters/ValueDataSource.ts index ab579487b8..41edc7da69 100644 --- a/packages/core/src/adapters/ValueDataSource.ts +++ b/packages/core/src/adapters/ValueDataSource.ts @@ -1180,7 +1180,9 @@ export class ValueDataSource implements DataSource { throw new Error(`ValueDataSource: Record with id "${id}" not found`); } this.items[index] = { ...this.items[index], ...data }; - this.emitMutation({ type: 'update', resource: _resource, id, record: { ...this.items[index] } }); + // The event carries the protocol's string id (objectui#10078): an inline + // item may hold a numeric key, and this adapter is where it converts. + this.emitMutation({ type: 'update', resource: _resource, id: String(id), record: { ...this.items[index] } }); return { ...this.items[index] }; } @@ -1190,7 +1192,7 @@ export class ValueDataSource implements DataSource { ); if (index === -1) return false; this.items.splice(index, 1); - this.emitMutation({ type: 'delete', resource: _resource, id }); + this.emitMutation({ type: 'delete', resource: _resource, id: String(id) }); return true; } diff --git a/packages/data-objectstack/src/index.ts b/packages/data-objectstack/src/index.ts index 0d32bac42e..8ba0378268 100644 --- a/packages/data-objectstack/src/index.ts +++ b/packages/data-objectstack/src/index.ts @@ -3908,7 +3908,9 @@ export class ObjectStackAdapter implements DataSource { data, opts?.ifMatch ? { ifMatch: opts.ifMatch } : undefined, ); - this.emitMutation({ type: 'update', resource, id, record: { ...result.record } }); + // String, as sent on the wire above (objectui#10078): the event states + // the protocol id, whatever this method's wider parameter admitted. + this.emitMutation({ type: 'update', resource, id: String(id), record: { ...result.record } }); this.notifyDroppedFields('update', resource, result, id, data as Record); return result.record; } catch (err) { @@ -3947,7 +3949,7 @@ export class ObjectStackAdapter implements DataSource { // stale — and this method, declared `Promise`, actually resolved // `undefined`. Following the rename is what restores both. if (result.success) { - this.emitMutation({ type: 'delete', resource, id }); + this.emitMutation({ type: 'delete', resource, id: String(id) }); } return result.success; } catch (err) { diff --git a/packages/types/src/__tests__/record-id-string-survivors-10078.test.ts b/packages/types/src/__tests__/record-id-string-survivors-10078.test.ts new file mode 100644 index 0000000000..56d84aee45 --- /dev/null +++ b/packages/types/src/__tests__/record-id-string-survivors-10078.test.ts @@ -0,0 +1,161 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#10078 - three more record-id declarations that the objectui#9511 + * ruling did not enumerate are strings too. + * + * ## What is pinned + * + * Director batch #195 item 1, letter A (objectui#9511) made a record id a + * `string` wherever METADATA names one, and extended it as "the same + * principle" to `CommentEntry.recordId` / `MentionNotification.recordId`. The + * three below are runtime / API shapes that ruling did not name; carrying the + * rule to them is the `domain:spec` seat's inheritance decision in the claim on + * objectui#10078. Each carries the same id with the old `string | number` + * spelling: + * + * - `CommentSearchResult.recordId` - the record a matching comment belongs to, + * copied from `CommentEntry.recordId` (already a string since objectui#9511); + * - `RecordSubscription.recordId` - the record a notification bell is for; + * - `DataSourceMutationEvent.id` - the record a `DataSource` write touched, the + * same id the `update` / `delete` doors already take as a string. + * + * All three are runtime / API shapes with NO zod mirror (nothing authors them + * into JSON), so the compiler is the only face that can refuse a number, and + * this file is where that refusal is asserted. + * + * ⚠️ This file pins these three ONLY. Other record ids in this package still + * admit a number and are out of its scope, so a green run here says nothing + * about the rest of the family. + * + * ## Where each row lives - these two halves measure different things + * + * The `type _...` rows and the `@ts-expect-error` controls are ERASED before + * vitest loads this file. `tsc -p packages/types/tsconfig.test.json` (chained + * from this package's `type-check` script) is the only thing that executes + * them: a green vitest run says nothing about them (objectui#3009). + * + * The `it(...)` rows are a source-text census over the declarations, which reds + * in a tree where `tsc` is never run and names the spelling it found. Each + * matcher is proven to fire on a synthetic wide spelling before an absence is + * believed. + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import type { CommentSearchResult, RecordSubscription } from '../views'; +import type { DataSourceMutationEvent } from '../data'; + +/* ------------------------------------------------------------------ * + * Compile-time half. + * ------------------------------------------------------------------ */ + +type Equal = + (() => T extends X ? 1 : 2) extends (() => T extends Y ? 1 : 2) ? true : false; +type Expect = T; + +type _CommentSearchResultRecordIdIsAString = Expect>; +type _RecordSubscriptionRecordIdIsAString = Expect>; +/** Optional member: `NonNullable` strips the `undefined` that `?` adds. */ +type _MutationEventIdIsAString = Expect, string>>; + +/** Stated separately so a degradation to `any` cannot pass the rows above. */ +type _CommentSearchResultRecordIdIsNotAny = Expect, false>>; +type _RecordSubscriptionRecordIdIsNotAny = Expect, false>>; +type _MutationEventIdIsNotAny = Expect, false>>; + +/** + * Value-site controls. Widen any member back and its directive becomes UNUSED + * (TS2578), so each control fires for exactly the change it exists to catch. + * The string spellings beside them must stay legal, or the refusals would + * prove nothing. + */ +// @ts-expect-error a numeric primary key is no longer a `CommentSearchResult.recordId` +const _numericSearchHit: CommentSearchResult = { comment: { id: 'c1', text: '', author: '', createdAt: '' }, objectName: 'account', recordId: 42 }; +// @ts-expect-error nor a `RecordSubscription.recordId` +const _numericSubscription: RecordSubscription = { recordId: 42, subscribed: true }; +// @ts-expect-error nor a `DataSourceMutationEvent.id` +const _numericMutation: DataSourceMutationEvent = { type: 'delete', resource: 'account', id: 42 }; + +const _stringSearchHit: CommentSearchResult = { comment: { id: 'c1', text: '', author: '', createdAt: '' }, objectName: 'account', recordId: '42' }; +const _stringSubscription: RecordSubscription = { recordId: '42', subscribed: true }; +const _stringMutation: DataSourceMutationEvent = { type: 'delete', resource: 'account', id: '42' }; +void [_numericSearchHit, _numericSubscription, _numericMutation, _stringSearchHit, _stringSubscription, _stringMutation]; + +/* ------------------------------------------------------------------ * + * Runtime half - a census over the declaration text. + * ------------------------------------------------------------------ */ + +const here = path.dirname(fileURLToPath(import.meta.url)); +// packages/types/src/__tests__ -> packages/types/src +const SRC = path.resolve(here, '..'); +const VIEWS_TEXT = readFileSync(path.join(SRC, 'views.ts'), 'utf8'); +const DATA_TEXT = readFileSync(path.join(SRC, 'data.ts'), 'utf8'); + +/** + * The declared type of `member` inside `export interface NAME ... { ... }`. + * The body is cut at the first line that closes the interface, so a member of + * the same name in a LATER interface cannot answer for this one. + */ +function memberType(source: string, iface: string, member: string): string | null { + const start = source.indexOf(`export interface ${iface}`); + if (start === -1) return null; + const end = source.indexOf('\n}', start); + const body = source.slice(start, end === -1 ? undefined : end); + const m = new RegExp(`\\n\\s*${member}\\??\\s*:\\s*([^;\\n]+);`).exec(body); + return m ? m[1].trim() : null; +} + +const SITES: ReadonlyArray = [ + ['views.ts', 'CommentSearchResult', 'recordId'], + ['views.ts', 'RecordSubscription', 'recordId'], + ['data.ts', 'DataSourceMutationEvent', 'id'], +]; + +const TEXT: Record = { 'views.ts': VIEWS_TEXT, 'data.ts': DATA_TEXT }; + +describe('the census instrument can fire (controls)', () => { + it('reads the wide spelling back off a synthetic declaration', () => { + const wide = 'export interface X {\n /** doc */\n recordId: string | number;\n other: boolean;\n}'; + expect(memberType(wide, 'X', 'recordId')).toBe('string | number'); + const wideOptional = 'export interface Y {\n id?: string | number;\n}'; + expect(memberType(wideOptional, 'Y', 'id')).toBe('string | number'); + }); + + it('does not answer for an interface from a member of a later one', () => { + const two = 'export interface A {\n x: boolean;\n}\n\nexport interface B {\n recordId: string | number;\n}'; + expect(memberType(two, 'A', 'recordId')).toBeNull(); + }); + + it('returns null rather than a false negative when the interface is absent', () => { + expect(memberType(VIEWS_TEXT, 'NoSuchInterface10078', 'recordId')).toBeNull(); + }); + + it('reads a member the objectui#9511 ruling already narrowed (lit control)', () => { + expect(memberType(VIEWS_TEXT, 'CommentEntry', 'recordId')).toBe('string'); + }); +}); + +describe('the three surviving record ids are strings (objectui#10078)', () => { + for (const [file, iface, member] of SITES) { + it(`\`${iface}.${member}\` declares the protocol string id`, () => { + expect( + memberType(TEXT[file], iface, member), + [ + `\`${iface}.${member}\` in ${file} declares a record id wider than \`string\`.`, + 'objectui#10078 carries the objectui#9511 record-id rule (a record id is a string', + 'wherever metadata names one) to this runtime shape; a host whose keys are', + 'numeric converts once, at its own adapter boundary.', + ].join('\n'), + ).toBe('string'); + }); + } +}); diff --git a/packages/types/src/data.ts b/packages/types/src/data.ts index 60d10d1c5d..b648dd356e 100644 --- a/packages/types/src/data.ts +++ b/packages/types/src/data.ts @@ -1171,8 +1171,13 @@ export interface DataSourceMutationEvent { resource: string; /** The affected record (present for create/update) */ record?: T; - /** The ID of the affected record (present for update/delete) */ - id?: string | number; + /** + * The ID of the affected record (present for update/delete). A string, per + * the one record-id rule stated on {@link DataSource} (objectui#9511, + * objectui#10078): an adapter whose backend keys are numeric converts at its + * own boundary before it emits, so no subscriber has to. + */ + id?: string; } /** diff --git a/packages/types/src/views.ts b/packages/types/src/views.ts index 37c0c627ff..2088cccd84 100644 --- a/packages/types/src/views.ts +++ b/packages/types/src/views.ts @@ -551,8 +551,8 @@ export interface CommentSearchResult { comment: CommentEntry; /** Object name the comment belongs to */ objectName: string; - /** Record ID the comment belongs to */ - recordId: string | number; + /** Record ID the comment belongs to. A string, per the one record-id rule (objectui#9511, objectui#10078). */ + recordId: string; /** Highlighted text snippet with search term marked */ highlight?: string; } @@ -727,8 +727,8 @@ export interface Reaction { * subscriptions and app billing — so none of them is a replacement. */ export interface RecordSubscription { - /** Record ID */ - recordId: string | number; + /** Record ID. A string, per the one record-id rule (objectui#9511, objectui#10078). */ + recordId: string; /** Whether the current user is subscribed */ subscribed: boolean; /** Notification channels */