fix: make instanceof reliable for ApiError, TimeoutError and CanceledPromiseError - #52
Merged
Conversation
…edPromiseError
These packages ship parallel CJS and ESM builds, and none of them declared an
`exports` map. A bundler could therefore resolve two distinct copies of the
same module — and therefore two distinct `ApiError` classes — in which case
`instanceof ApiError` silently returned `false` for an error thrown by the
other copy.
There was no fallback either: the constructors never assigned `name`, so
`e.name` was `"Error"` and could not be used to discriminate.
This fails *silently*: the guard compiles, type-narrows, and simply never
matches, so an error handler looks correct while falling through to a 500.
Error identity
------------------------------------------------------------------------
`ApiError`, `TimeoutError` and `CanceledPromiseError` now brand their
instances with a symbol from the cross-realm global registry
(`Symbol.for('@datocms/rest-client-utils:<Name>')`) and implement
`static [Symbol.hasInstance]`, making `instanceof` structural rather than
identity-based. The registry is shared across module copies and realms, so the
check holds wherever the error came from. Subclasses fall back to a real
prototype-chain check, so they stay exact.
Consumers need no code changes — plain `instanceof` just works now.
Each constructor also sets `name` ('ApiError', 'TimeoutError',
'CanceledPromiseError'), which was previously left as the inherited "Error".
Verified with two genuinely separate copies of the module loaded at once:
`instanceof` is now true across copies for all three classes, while a plain
`Error` — and an impostor that merely sets `name = 'ApiError'` — are still
correctly rejected.
Stack trace fix
------------------------------------------------------------------------
`TimeoutError`'s constructor called `Error.captureStackTrace(this, ApiError)`,
trimming the stack at the wrong constructor. Now passes `TimeoutError`.
Packaging
------------------------------------------------------------------------
Added `exports` maps to the seven packages that lacked one, using the map
already present in `cma-client-node` as the template. Their ESM builds already
use `.js` specifiers and already emit a `{"type":"module"}` marker, so they are
genuinely loadable — the map just makes resolution explicit instead of leaving
it to each bundler's `mainFields` ordering.
The maps deliberately keep `./dist/*`, `./src/*`, `./resources.json` (where
published) and `./package.json` reachable, so existing deep imports keep
working. One caveat: `exports` targets do not get Node's extension guessing, so
a deep import must now spell out the extension.
`cma-client-analysis` and `cma-client-node` already had maps and are untouched.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These packages ship parallel CJS and ESM builds, and none of them declared an
exportsmap. A bundler could therefore resolve two distinct copies of the same module — and therefore two distinctApiErrorclasses — in which caseinstanceof ApiErrorsilently returnedfalsefor an error thrown by the other copy:There was no fallback: the constructors never assigned
name, soe.namewas the inherited"Error"and could not be used to discriminate.This fails silently — the guard compiles, type-narrows, and simply never matches, so an error handler looks correct while falling through to a 500.
Error identity
ApiError,TimeoutErrorandCanceledPromiseErrornow brand their instances with a symbol from the cross-realm global registry (Symbol.for('@datocms/rest-client-utils:<Name>')) and implementstatic [Symbol.hasInstance], makinginstanceofstructural rather than identity-based. The registry is shared across module copies and realms, so the check holds wherever the error came from. Subclasses fall back to a real prototype-chain check so they stay exact.Consumers need no code changes — plain
instanceofjust works now.Each constructor also sets
name('ApiError','TimeoutError','CanceledPromiseError').Verification
With two genuinely separate copies of the module loaded at once:
11 regression tests added in
packages/rest-client-utils/src/__tests__/errors.test.ts, covering the prototype chain, cross-copy recognition, subclass exactness, cross-type confusion, and rejection of unrelated values (including an impostor error that merely setsname = 'ApiError').Stack trace fix
TimeoutError's constructor calledError.captureStackTrace(this, ApiError)— the wrong constructor, so the stack was trimmed at the wrong frame. Now passesTimeoutError.Packaging
Added
exportsmaps to the seven packages that lacked one, using the map already present incma-client-nodeas the template. Their ESM builds already use.jsspecifiers and already emit a{"type":"module"}marker, so they are genuinely loadable — the map just makes resolution explicit instead of leaving it to each bundler'smainFieldsordering.The maps deliberately keep
./dist/*,./src/*,./resources.json(where published) and./package.jsonreachable, so existing deep imports keep working:One caveat:
exportstargets don't get Node's extension guessing, so a deep import must now spell out the extension (.../errors.js, not.../errors).cma-client-analysisandcma-client-nodealready had maps and are untouched.Testing notes
npx biome ci packagesis clean andnpm run buildsucceeds for all 9 packages. I ran the newerrors.test.tssuite (11 passing) but not the fullnpm test, since the rest of the suite provisions and deletes real DatoCMS projects against a live account — worth a CI run before merging.Release note
Worth shipping as a minor, not a patch: the
exportsmaps change resolution.