Generate TypeScript error-code declarations from the registry - #357
Conversation
ee284fd to
b0ae20c
Compare
The AIT error codes are now registered, so pin ably-common at the merge of ably/ably-common#353. That brings in 104009, 104012 and 104013, which the previous commit's enum referred to before they existed, and renames 104003's identifier to run_lifecycle_event_publish_failed. protocol/errors.json no longer maps a code straight to a description string. Entries now sit under a "codes" envelope, each an object with an identifier, title and summary. validate-error-codes.ts indexed the top level, so against the bumped pin it would have found nothing and reported all 19 codes as missing - a misleading failure rather than a clean pass. It now reads codes and prints each entry's identifier, and reports an absent envelope as a stale or uninitialised submodule instead of letting it read as 19 unregistered codes. The check compares codes, not names. Generating the constants from the registry (ably/ably-common#357) is what makes the registry identifier the one spelling every SDK uses, and replaces this script. [AIT-1259] Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
VeskeR
left a comment
There was a problem hiding this comment.
LGTM overall. Only one genuine request below re running the scripts on Windows machine, and couple of nits below - up to you
Something pre-existing Claude flagged and I tested:
npm run generate:errorcodes-ts -- --format=type fails with:
> [email protected] generate:errorcodes-ts
> node errors/scripts/generate-ts.js --format=type
10000.md: missing opening `---` frontmatter fence
on Windows machines, presumably because:
frontmatter.js:17 errors/scripts/frontmatter.js
function parseFrontmatter(content) {
if (!content.startsWith('---\n')) {
return { error: 'missing opening---frontmatter fence' };With Git for Windows' default core.autocrlf=true and no .gitattributes, errors/codes/*.md land as CRLF, so every file fails that fence check. This is my actual output on the branch, and 6 of the 25 tests fail with it:
and the is to change it to:
function parseFrontmatter(rawContent) {
const content = rawContent.replace(/\r\n/g, '\n');
I think it's worth fixing here (as a separate commit) so that it runs on every machine - we have people developing on Windows for some SDKs, so a good idea to make it work for them too.
Two review points from #357. `docBlock` wrapped the summary but interpolated the title directly, so a title inside the 10-word guideline could still overrun the doc width: 40182 emitted an 85-column line. Titles now go through `wrap` as well, which takes the widest line in the generated output down to 79. The CLI test named for an unusable registry ran `--format=enum` instead, and its comment said as much: a bad format throws the same class from the same `try`/`catch`, but never reaches `nameEntries`, where all three registry assertions live. So nothing verified the promise that a duplicate identifier prints a message rather than a stack trace. Since the script resolves the registry relative to itself, the test now stands a copy of the script up over a registry that genuinely has a duplicate, and the coverage the old test did have keeps an accurate name. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
e171ef4 to
ae180b4
Compare
|
fixed frontmatter parsing broken on Windows in ae180b4 |
VeskeR
left a comment
There was a problem hiding this comment.
Approving as I've addressed my feedback and otherwise is happy with a PR 👍
The JS SDKs each name error codes themselves, and the names have drifted from this registry: 15 of ably-chat-js's 20 disagree with the code's `identifier`, and ably-js uses bare numeric literals at ~149 call sites. Generating the names instead makes `identifier` the single spelling every SDK uses, and makes registering a code here the only step needed before referring to it. errors/scripts/generate-ts.js emits one of two shapes. `--format=type` gives a bare `ErrorCode` union of numeric literals, for a consumer that wants compile-time checking at no bundle-size cost — the type erases, and because registry codes are 5-6 digits while HTTP statuses are 3, a `code` and `statusCode` transposed at a call site fails to compile. `--format=const` gives one documented `export const` per code plus the union, for consumers that need the values at runtime; individual consts rather than an aggregate object or a TS `enum` so that unused codes tree-shake out of the browser bundles those SDKs ship. Nothing generated is committed here. Each consuming repository generates from the submodule it already vendors, commits the output into its own src/, and regenerates in CI at the pinned commit to fail on a diff. That mirrors how ably/docs publishes the error pages, and means an SDK cannot merge a reference to a code that is not merged here first. The generator refuses to emit rather than produce an unusable file, on a duplicate `identifier`, two identifiers colliding on one PascalCase name, or a name that is not a valid JavaScript identifier. None of the three occurs in the registry today. Those failures, a bad argument, and an unwritable `--out` all print as a plain message and exit 1; a stack trace there would only be noise, so one now indicates a bug in the generator rather than a problem with the input. Output is sorted by numeric code and byte-identical for a given registry state, as the drift check requires, and depends only on Node's standard library and the local frontmatter parser, so it runs from a superproject with no `npm install` inside the submodule. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
`parseFrontmatter` tested `content.startsWith('---\n')`, so on a checkout made
with git's default `core.autocrlf=true` every `codes/*.md` arrives with CRLF
endings and fails the opening fence check. All three registry scripts were
affected: `validate:errors`, `generate:errors` and `generate:errorcodes-ts` each
reported the entire registry as malformed, which reads as corruption rather than
as a line-ending problem. That was tolerable while these scripts only ran here
and in ably/docs CI, both on Linux, but the TypeScript generator is meant to be
run by SDK contributors on their own machines and some of them develop on
Windows, so the failure now sits on the documented path. Normalising in the
parser rather than adding a `.gitattributes` fixes it however the bytes arrived,
including for anyone who already has a CRLF working copy. The only behaviour
that changes is the fence check: values were already `trimEnd()`ed, and
`protocol/errors.json` regenerates byte-identically. The tests build both
endings in memory, so they assert the same thing on Linux, macOS and Windows
regardless of how git checked the repository out.
`docBlock` wrapped the summary but interpolated the title directly, so a title
inside the 10-word guideline could still overrun the doc width: 40182 emitted an
85-column line. Titles now go through `wrap` as well, which takes the widest
line in the generated output down to 79, and a check over the whole committed
registry keeps it there.
The CLI test named for an unusable registry ran `--format=enum` instead, and its
comment said as much: a bad format throws the same class from the same
`try`/`catch`, but never reaches `nameEntries`, where all three registry
assertions live. So nothing verified the promise that a duplicate identifier
prints a message rather than a stack trace. Since the script resolves the
registry relative to itself, the test now stands a copy of the script up over a
registry that genuinely has a duplicate, and the coverage the old test did have
keeps an accurate name.
Finally, `generate-ts.js` sat alongside `generate-errors-json.js` and
`validate-errors.js` and read as though it generated TypeScript for the
repository at large rather than the error-code declarations specifically, so it
now agrees with the npm script that runs it.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
ae180b4 to
9fbd6d2
Compare
`new ErrorInfo(message, code, statusCode)` takes two adjacent numbers the
compiler accepted in either order, and codes were written as bare literals
with nothing tying them to the registry in ably-common. Generating the set
of registered codes from that registry and narrowing `code` to it closes
both gaps: an unregistered code and a transposed `code`/`statusCode` pair
now fail to compile, because registry codes are 5-6 digits and an HTTP
status is 3, so a status can never be a member of the union.
src/common/lib/types/errorcodes.ts is generated by the ably-common script
`errors/scripts/generate-ts.js` via `npm run generate:errorcodes-ts`, and
committed. The type erases at compile time, so this costs nothing in the
bundle: `errorcodes` appears zero times in build/ably-node.js and
build/ably.min.js.
Errors this SDK raises are checked; errors decoded from the server are not,
because the server chooses those codes and may use ones a given client
version does not know. Rather than leave one signature to serve both, the
two are separated by name:
fromValues - checked, for errors this repository raises
fromWireValues - unchecked, for a response body, a response header, or
the `error` field of a ProtocolMessage
`fromValues` was previously reached by both kinds of caller, so it could
not be both checked and honest. Splitting it makes the safe path the
default and leaves every unchecked site greppable; there are ten, all
genuine server decodes apart from the application's authCallback in auth.ts,
whose code originates in user code and so cannot be validated here.
`ErrorInfoValues` and `PartialErrorInfoValues` are the checked shapes for
the options-object constructor. `IConvertibleToErrorInfo` keeps
`code: number` and is now reached only by the wire path.
This exposed three defects:
- Five sites had `code` and `statusCode` the wrong way round:
src/common/types/http.ts (x2), nodejs and web crypto.ts, and web
http.ts. Their `err.code` and `err.statusCode` were transposed and are
now correct. The web crypto site had no valid pairing in either order
(400/50000); it now matches its corrected Node sibling.
- connectionerrors.ts `unknownChannelErr` used UNKNOWN_CONNECTION_ERR
(50002), which is why UNKNOWN_CHANNEL_ERR (50001) was unused. The
registry gives 50001 as `Internal channel error` and protocol.ts
already uses it that way. Channel errors now report 50001.
- normaliseAuthcallbackError in auth.ts took its code off an `any`, so
neither the callback's value nor the SDK's own fallbacks were checked.
The fallbacks are now annotated constants; the callback's value is an
explicit cast.
Note the first two change observable error codes.
ConnectionErrorCodes uses `as const satisfies Record<string, ErrorCode>`
so that keys stay literal and values are checked. A plain
`Record<string, ErrorCode>` annotation would have kept accepting a
misspelled key, which is the shape of the bug fixed above.
Two CI steps in check.yml. The first typechecks src/, which nothing did
before: the build is esbuild and the UTS suite runs under tsx, both of
which strip types without checking them, so `tsc --noEmit ably.d.ts
modular.d.ts` was only covering the two declaration files. Without it none
of this is enforced anywhere but an editor. It passes with no other change.
The second regenerates at the pinned submodule commit and fails on a diff,
so a reference to a code that is not registered in ably-common cannot merge
here first.
The submodule pin moves to ably-common main, which the generate and
drift-check steps need: the previous pin predated errors/codes/ existing.
Only protocol/ differs between the two; test-resources/, which is all
ably-js reads from the submodule, is unchanged.
The registry has 276 codes at this pin, including the 104xxx block for
ably-ai-transport-js. The generator is errors/scripts/generate-errorcodes-ts.js
there, renamed from generate-ts.js during review of ably/ably-common#357.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The AIT error codes are now registered, so pin ably-common at the merge of ably/ably-common#357, which carries the codes added by #353 alongside the error-code generator. That brings in 104009, 104012 and 104013, which the previous commit's enum referred to before they existed, and renames 104003's identifier to run_lifecycle_event_publish_failed. protocol/errors.json no longer maps a code straight to a description string. Entries now sit under a "codes" envelope, each an object with an identifier, title and summary. validate-error-codes.ts indexed the top level, so against the bumped pin it would have found nothing and reported all 19 codes as missing - a misleading failure rather than a clean pass. It now reads codes and prints each entry's identifier, and reports an absent envelope as a stale or uninitialised submodule instead of letting it read as 19 unregistered codes. The check compares codes, not names. The pin also brings in errors/scripts/generate-errorcodes-ts.js, which derives the constants from the registry so that the identifier is the one spelling every SDK uses. Adopting it here replaces this script, and is left to its own change: it renames the members this branch does not touch, so it is a breaking change to the exported ErrorCode enum in its own right. [AIT-1259] Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The AIT error codes are now registered, so pin ably-common at the merge of ably/ably-common#357, which carries the codes added by #353 alongside the error-code generator. That brings in 104009, 104012 and 104013, which the previous commit's enum referred to before they existed, and renames 104003's identifier to run_lifecycle_event_publish_failed. protocol/errors.json no longer maps a code straight to a description string. Entries now sit under a "codes" envelope, each an object with an identifier, title and summary. validate-error-codes.ts indexed the top level, so against the bumped pin it would have found nothing and reported all 19 codes as missing - a misleading failure rather than a clean pass. It now reads codes and prints each entry's identifier, and reports an absent envelope as a stale or uninitialised submodule instead of letting it read as 19 unregistered codes. The check compares codes, not names. The pin also brings in errors/scripts/generate-errorcodes-ts.js, which derives the constants from the registry so that the identifier is the one spelling every SDK uses. Adopting it here replaces this script, and is left to its own change: it renames the members this branch does not touch, so it is a breaking change to the exported ErrorCode enum in its own right. [AIT-1259] Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Summary
Adds
errors/scripts/generate-ts.js, which turns the registry inerrors/codes/into TypeScript error-code declarations for the JavaScript SDKs. Nothing generated is committed here — this PR adds the generator, its tests, and thegenerate:errorcodes-tsscript.The JS SDKs currently name error codes themselves, and the names have drifted from the registry: 15 of
ably-chat-js's 20 disagree with the code'sidentifier, andably-jsuses bare numeric literals at ~149 call sites (five of which hadcodeandstatusCodetransposed). Generating the names makesidentifierthe single spelling every SDK uses, and makes registering a code here the only step needed before referring to it — which is also whyidentifieris a frozen contract rather than something to churn.The two output formats
--format=typeemits a bare union of numeric literals:This is for
ably-js, which gates PRs on bundle size. The type erases at compile time, so it costs zero bytes, and existing numeric literals already satisfy the union — so all ~149 call sites keep compiling untouched while gaining the checking. Because registry codes are 5–6 digits and HTTP statuses are 3, a transposedcode/statusCodepair is caught exactly rather than heuristically.--format=constemits one documented constant per code, then the union:This is for the wrapper SDKs, which need the values at runtime. Individual consts rather than an aggregate object or a TS
enum, because object properties and enum members don't tree-shake — a 260-member enum would land whole in the bundles those SDKs ship to browsers. The JSDoc carries the registrytitle,summary, and help link, so hovering a constant in an editor shows what the code means.How consumers use it
Each SDK runs the generator against the
ably-commonsubmodule it already vendors, commits the output into its ownsrc/, and adds a CI step that regenerates at the pinned commit and diffs — the same arrangement asably/docsand its error pages. Two properties that buys: a developer can register a code and use it without publishing anything, and because CI regenerates at the pinned commit, an SDK can't merge a reference to a code that isn't merged here first.Output is sorted by numeric code and byte-identical for a given registry state, as that drift check requires, and depends only on Node's standard library plus the local
frontmatter.js— so it runs from a superproject with nonpm installinside the submodule.Failure behaviour
The generator refuses to emit rather than produce an unusable file, on a duplicate
identifier, two identifiers colliding on one PascalCase name, or a name that isn't a valid JavaScript identifier. None of the three occurs in the registry today; the assertions are to keep it that way.Those failures, a bad argument, an absent
errors/codes/, and an unwritable--outall print as a plain message and exit 1. A stack trace there would only be noise, so one now indicates a bug in the generator rather than a problem with the input. The absent-registry message calls out an uninitialised or stale submodule specifically, sinceably-js's pin predateserrors/codes/existing and that's the first thing anyone adopting this there will hit.Verification
tsc --strict.const d: ErrorCode = 400(a transposed HTTP status) and an unregistered12345are both rejected, which is the propertyably-jswants. Not wired into the test suite, since TypeScript isn't a dependency here.ably-chat-jsandably-ai-transport-jsare produced exactly as tabulated.The
.eslintrc.jschange belongs to the tests: they assert through anexpectCleanFailurehelper, whichjest/expect-expectreads as a test with no assertions, soassertFunctionNamesis configured fortest/**rather than disabling the rule.Follow-ups, not in this PR
ably-jsneeds its pin moved off496da5e, which predateserrors/codes/.ably-chat-jsandably-ai-transport-jsare breaking, since both exportErrorCodeas a TSenumfrom their public API — one major, or@deprecatedaliases for a release.isErrorCode(n): n is ErrorCodeguard for narrowing a relayed wire code. It needs the full set as runtime data, so it can't be part of thetypeoutput.errors/scripts/generate-errors-json.jshas the stack-trace-on-registry-fault behaviour that this generator now avoids; worth aligning separately.🤖 Generated with Claude Code