You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Mark execute() as async on every request class, so that all failures arrive as a rejected promise.
This is a breaking change. See "Impact on consumers" below.
Why
execute() returns a promise today, because it ends with return webHelper.get(path). It is not declared async, however, so validation errors throw synchronously before any promise exists. The contract is therefore mixed: validation errors throw, while HTTP errors reject.
That makes idiomatic promise code wrong:
client.images().execute().then(handleResult).catch(handleError);// never sees "must specify at least one image id"
The caller gets an uncaught exception rather than a rejection. Marking execute() as async makes every failure a rejection, which is what a caller expects from a promise-returning method.
Affected throw sites
Six files throw synchronously inside execute():
lib/customrequest.js — "must specify a route", and "No appropriate HTTP method found for this request."
lib/downloadsimages.js — "must specify an image id"
lib/downloadsvideos.js — "must specify a video id"
lib/events.js — "must specify at least one event id"
lib/images.js — "must specify at least one image id"
lib/videos.js — "must specify at least one video id"
The remaining eight execute() methods have no validation guard, so async is cosmetic there and only makes the contract uniform.
The client constructor also throws SdkException for a missing apiKey. That one stays synchronous, because a constructor cannot reject.
Impact on consumers
Code that catches a validation error with try/catch directly around the call stops working:
try{client.images().execute();// after this change: returns a rejected promise, does not throw}catch(e){/* no longer runs */}
The failure mode is silent. There is no error at the call site, just an unhandled rejection elsewhere. Ship it as a major version and call it out in the release notes.
Two things limit the blast radius:
Every one of these guards a missing required argument, so they fire during development rather than in production against valid input.
Any consumer already handling both failure paths correctly must already have a .catch(), and the rejection lands there.
Work
Add async to the 14 execute() methods across lib/.
Add tests for the six throw sites, asserting rejection. There is currently no coverage of these paths, so nothing would catch a regression. Write the tests first, against the new behavior.
Update gettyimages-api.d.ts. The execute(): Promise<any> signatures do not change, however the doc comment on SdkException states that it "is thrown synchronously from execute() and from the client constructor". After this change it is thrown synchronously only from the constructor.
Update the README error-handling guidance, if any example shows a synchronous try/catch around execute().
Sequencing
Do this second in the milestone, after #85 and before #64.
Convert the source to TypeScript #85 (TypeScript conversion) should come first. This change alters the contract of 14 execute() methods, and the compiler verifies it landed consistently across all of them.
Support both ESM and CommonJS #64 (dual ESM and CommonJS) should come after. It packages the public surface, so that surface should settle first.
Notes
async is a function-level keyword and works identically in CommonJS and ESM. This issue therefore does not depend on Support both ESM and CommonJS #64, and introduces no top-level await.
lib/credentials.js already uses async for getAccessToken() and refreshAccessToken(), so the style is established in the codebase.
Mark
execute()asasyncon every request class, so that all failures arrive as a rejected promise.This is a breaking change. See "Impact on consumers" below.
Why
execute()returns a promise today, because it ends withreturn webHelper.get(path). It is not declaredasync, however, so validation errors throw synchronously before any promise exists. The contract is therefore mixed: validation errors throw, while HTTP errors reject.That makes idiomatic promise code wrong:
The caller gets an uncaught exception rather than a rejection. Marking
execute()asasyncmakes every failure a rejection, which is what a caller expects from a promise-returning method.Affected throw sites
Six files throw synchronously inside
execute():lib/customrequest.js— "must specify a route", and "No appropriate HTTP method found for this request."lib/downloadsimages.js— "must specify an image id"lib/downloadsvideos.js— "must specify a video id"lib/events.js— "must specify at least one event id"lib/images.js— "must specify at least one image id"lib/videos.js— "must specify at least one video id"The remaining eight
execute()methods have no validation guard, soasyncis cosmetic there and only makes the contract uniform.The client constructor also throws
SdkExceptionfor a missingapiKey. That one stays synchronous, because a constructor cannot reject.Impact on consumers
Code that catches a validation error with
try/catchdirectly around the call stops working:The failure mode is silent. There is no error at the call site, just an unhandled rejection elsewhere. Ship it as a major version and call it out in the release notes.
Two things limit the blast radius:
.catch(), and the rejection lands there.Work
asyncto the 14execute()methods acrosslib/.gettyimages-api.d.ts. Theexecute(): Promise<any>signatures do not change, however the doc comment onSdkExceptionstates that it "is thrown synchronously fromexecute()and from the client constructor". After this change it is thrown synchronously only from the constructor.try/catcharoundexecute().Sequencing
Do this second in the milestone, after #85 and before #64.
execute()methods, and the compiler verifies it landed consistently across all of them.Notes
asyncis a function-level keyword and works identically in CommonJS and ESM. This issue therefore does not depend on Support both ESM and CommonJS #64, and introduces no top-levelawait.lib/credentials.jsalready usesasyncforgetAccessToken()andrefreshAccessToken(), so the style is established in the codebase.