Skip to content

Expose execute() as async #65

Description

@mapitman

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions