|
| 1 | +--- |
| 2 | +"@objectstack/client": major |
| 3 | +"@objectstack/cli": patch |
| 4 | +--- |
| 5 | + |
| 6 | +fix(client)!: `DeleteDataResult` declares the schema it names — `success`, not `deleted` (#5638) |
| 7 | + |
| 8 | +`DeleteDataResult` — the return type of `client.data.delete()` and of the |
| 9 | +project-scoped `client.project(id).data.delete()` — carried the comment |
| 10 | +`Spec: DeleteDataResponseSchema` above a declaration that contradicted it: |
| 11 | + |
| 12 | +```ts |
| 13 | +/** Spec: DeleteDataResponseSchema */ |
| 14 | +export interface DeleteDataResult { |
| 15 | + object: string; |
| 16 | + id: string; |
| 17 | + deleted: boolean; // ← the schema declares `success` |
| 18 | +} |
| 19 | +``` |
| 20 | + |
| 21 | +`DeleteDataResponseSchema` (`packages/spec/src/api/protocol.zod.ts`) declares |
| 22 | +`{ object, id, success }`. `deleted` has never been declared by any schema, and |
| 23 | +no server path has ever returned it on `/data/:object/:id`. |
| 24 | + |
| 25 | +**The old key was never readable at runtime — this rename reveals a defect, it |
| 26 | +does not break working code.** Both `delete` surfaces are pure `unwrapResponse` |
| 27 | +/ `_unwrap` passthroughs: the SDK returns the server's body untouched, so this |
| 28 | +interface is a *claim* about the wire, never a rewrite of it. The claim was |
| 29 | +false in the one direction that matters — the compiler endorsed the wrong |
| 30 | +spelling: |
| 31 | + |
| 32 | +```ts |
| 33 | +const r = await client.data.delete('task', id); |
| 34 | +if (r.deleted) { … } // compiled; `undefined` at runtime; branch never taken |
| 35 | +if (r.success) { … } // rejected by the compiler; correct on the wire |
| 36 | +``` |
| 37 | + |
| 38 | +## What to change |
| 39 | + |
| 40 | +`r.deleted` → `r.success`. That is the whole migration. Nothing about the |
| 41 | +request, the route, the status codes or the error shapes changes, and no server |
| 42 | +needs upgrading: the value you are now allowed to read is the one that was |
| 43 | +already arriving. |
| 44 | + |
| 45 | +⛔ **Do not write `r.success ?? r.deleted`.** There is one producer shape, and a |
| 46 | +consumer that accepts two spellings is the shape contract-first exists to |
| 47 | +prevent — the same ruling #5581 applied on the producer side. No deprecated |
| 48 | +`deleted?: boolean` transition key ships for the same reason; a transition |
| 49 | +period is for keys that *worked*, and this one never did. |
| 50 | + |
| 51 | +## Why the type was wrong on every deployment, not just some |
| 52 | + |
| 53 | +The protocol path (`deleteData`) has always answered `success`. #5581 / PR |
| 54 | +#5641 brought the ObjectQL fallback — the path a slim assembly without |
| 55 | +`MetadataPlugin` takes — to the same shape. So before that fix the declaration |
| 56 | +was wrong on ordinary deployments and accidentally right on slim ones; after |
| 57 | +it, both paths answer `{ object, id, success }` and the declaration was simply |
| 58 | +wrong everywhere. The consumer-side correction had to follow the producer, not |
| 59 | +lead it. |
| 60 | + |
| 61 | +## `os data delete` was reading the phantom key too |
| 62 | + |
| 63 | +`packages/cli/src/commands/data/delete.ts` built its `--format json` / `--format |
| 64 | +yaml` payload with `deleted: result.deleted`. That evaluated to `undefined`, and |
| 65 | +`JSON.stringify` drops undefined values — so the `deleted` key the command has |
| 66 | +always declared **never appeared in a single run**. It now carries |
| 67 | +`result.success`, the server's own verdict. |
| 68 | + |
| 69 | +Observable change: `os data delete --format json` gains `deleted: true` (YAML |
| 70 | +likewise) on a successful delete. The key name stays `deleted` deliberately — |
| 71 | +it is the CLI's output key, not the protocol's, and the payload's top-level |
| 72 | +`success` already means something different (the CLI envelope's "the command |
| 73 | +completed"). Conflating the two is the hazard #5641 called out when it noted |
| 74 | +that `body.success` and `body.data.success` are different facts. Scripts |
| 75 | +reading `.deleted` from this command were reading `undefined` before and get a |
| 76 | +boolean now; nothing that worked stops working. |
| 77 | + |
| 78 | +## Downstream |
| 79 | + |
| 80 | +`objectui`'s `ObjectStackDataSource.delete()` is a live victim of the old |
| 81 | +declaration — it guards `emitMutation` on `result.deleted`, so the delete |
| 82 | +mutation event has never fired against a real server and the method returns |
| 83 | +`undefined` where it declares `boolean`. Its own suite stayed green because the |
| 84 | +fixture mocks `{ deleted: true }`, a body no server produces. Filed as |
| 85 | +objectstack-ai/objectui#3412, which is blocked on this package publishing — |
| 86 | +its fix is a type unblock, not a behaviour change, since `success` is already |
| 87 | +what arrives. |
| 88 | + |
| 89 | +## Pins |
| 90 | + |
| 91 | +`packages/client/src/data-delete-result-shape.test.ts` asserts mutual |
| 92 | +assignability between `DeleteDataResult` and the spec's `DeleteDataResponse`, |
| 93 | +so a rename on either side (or a re-added optional `deleted`) fails |
| 94 | +`check:test-typecheck`. `client.hono.test.ts` gains the delete case this live |
| 95 | +server suite never had: a real DELETE over HTTP whose body is read as |
| 96 | +`deleted.success` and whose key set is asserted literally — `z.object` strips |
| 97 | +unknown keys, so a passing parse alone cannot prove no stray `deleted` rode |
| 98 | +along. |
0 commit comments