feat: add ArtifactsError type definitions#6710
feat: add ArtifactsError type definitions#6710dmmulroy wants to merge 2 commits intocloudflare:mainfrom
Conversation
Add typed error surface for the Artifacts binding: - ArtifactsErrorCode: union of all error code strings - ArtifactsError: extends Error with readonly code, numericCode - @throws annotations on all Artifacts and ArtifactsRepo methods Follows the same interface-extends-Error pattern used by StreamError, ImagesError, and MediaError. Note: generated-snapshot/ needs regeneration via `just generate-types`.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6710 +/- ##
==========================================
- Coverage 66.57% 66.55% -0.02%
==========================================
Files 402 402
Lines 115911 115856 -55
Branches 19407 19406 -1
==========================================
- Hits 77167 77110 -57
+ Misses 27165 27161 -4
- Partials 11579 11585 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/bonk review this PR |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI-generated review (by OpenCode). May contain inaccuracies — please verify.
Summary: The types look well-structured and follow existing Cloudflare binding error patterns. A few items to address:
- The
generated-snapshot/files should not be hand-edited — CI should regenerate them. The manual edits will likely be overwritten and may cause the snapshot CI check to fail untiljust generate-typesis run. - Several error codes in
ArtifactsErrorCodeare never referenced in any@throwsannotation. Either they can be thrown (and should be documented) or they're unused. - Minor: the
import()method documentsINVALID_INPUTfor bad URLs, but a more specificINVALID_URLcode exists in the union and seems like a better fit.
| type ArtifactsErrorCode = | ||
| | 'ALREADY_EXISTS' | ||
| | 'NOT_FOUND' | ||
| | 'IMPORT_IN_PROGRESS' | ||
| | 'FORK_IN_PROGRESS' | ||
| | 'INVALID_INPUT' | ||
| | 'INVALID_REPO_NAME' | ||
| | 'INVALID_TTL' | ||
| | 'INVALID_URL' | ||
| | 'REMOTE_AUTH_REQUIRED' | ||
| | 'UPSTREAM_UNAVAILABLE' | ||
| | 'MEMORY_LIMIT' | ||
| | 'INTERNAL_ERROR'; |
There was a problem hiding this comment.
Five of the twelve error codes — INVALID_URL, REMOTE_AUTH_REQUIRED, UPSTREAM_UNAVAILABLE, MEMORY_LIMIT, INTERNAL_ERROR — are defined in the union but never appear in any @throws annotation. A few questions:
- Can any method throw
INTERNAL_ERROR? If so, it might be worth adding a blanket note (e.g., on theArtifactsinterface JSDoc) that any method may throw withINTERNAL_ERROR. INVALID_URLseems like a natural fit forimport()— isINVALID_INPUTused there instead, or can both be thrown?REMOTE_AUTH_REQUIREDandUPSTREAM_UNAVAILABLEpresumably relate toimport()orfork()with external remotes — consider documenting those.
Having undocumented codes in the union is fine if they're needed for instanceof/code matching, but the discoverability is much better with @throws annotations.
| * @param params Source URL and optional branch/depth, plus target name and options. | ||
| * @returns Repo metadata with initial token. | ||
| * @throws {ArtifactsError} with code `INVALID_REPO_NAME` if the target name is invalid. | ||
| * @throws {ArtifactsError} with code `INVALID_INPUT` if the source URL is not valid HTTPS. |
There was a problem hiding this comment.
INVALID_INPUT is documented here for invalid source URLs, but the ArtifactsErrorCode union includes a more specific INVALID_URL code. Is INVALID_INPUT the code actually returned by the runtime for bad URLs, or should this be INVALID_URL?
| }, | ||
| ): Promise<ArtifactsCreateRepoResult>; | ||
| } | ||
| // ── Error types ────────────────────────────────────────────────────────────── |
There was a problem hiding this comment.
Per repo convention (types/AGENTS.md, AGENTS.md): files in generated-snapshot/ should not be edited directly — they are overwritten by just generate-types. The correct workflow is to change only types/defines/artifacts.d.ts and let CI regenerate the snapshots (or run just generate-types locally).
The PR description mentions a local bazel cache issue prevented running generation. That's understandable, but the hand-edited snapshots may diverge from what the generator actually produces (e.g., quote style, blank line handling, comment formatting). CI should flag this; if it does, the snapshot commit can be dropped and regenerated in CI.
|
Review posted successfully. Here's a summary of the feedback I left on PR #6710: 3 inline comments:
|
3 participants
What
Add
ArtifactsErrorandArtifactsErrorCodeTypeScript type definitions to the Artifacts binding.Changes
types/defines/artifacts.d.ts:ArtifactsErrorCode— union of all error code strings (ALREADY_EXISTS,NOT_FOUND,INVALID_INPUT, etc.)ArtifactsError— extendsErrorwithreadonly code: ArtifactsErrorCodeandreadonly numericCode: number@throws {ArtifactsError}annotations on allArtifactsandArtifactsRepomethodsFollows the same
interface extends Errorpattern used byStreamError,ImagesError, andMediaError.Snapshot
generated-snapshot/needs regeneration — CI will flag the diff and produce the updated snapshot artifact. I was unable to runjust generate-typeslocally due to a bazel cache issue unrelated to this change.