Problem/Motivation

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork api_client-3407051

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pratik_kamble created an issue. See original summary.

coby.sher’s picture

I might pick this up. I'm working on https://www.drupal.org/project/api_client/issues/3376946 and finding a few places where it would be nice to throw and error, but this breaks a lot of the current tests. So instead of gilding that one I will start on this.

Here are my thoughts on what the API should look like:

We need to be able to return an error from `fetch`. We can change the return type to a tuple like so:

async fetch(
    input: RequestInfo | URL,
    init?: RequestInit,
  ): Promise<[Response | null, Error | null]>

This has some advantages and some disadvantages: any time `fetch` is called, the result would need to be a destructured array, and we will have to check if Error is not null. This is similar to Golang's convention which can feel kind of verbose, but does enforce error checking and handling.

We could also use an object for this but that detail isn't super important to me. The important part is actually returning and not throwing the error from `fetch`, allowing us to throw or handle the error as the end user sees fit.

Thoughts?

pratik_kamble’s picture

@coby.sher Make sense to me. Considering 5XX will be treated as normal response only.

brianperry’s picture

What you outlined makes sense to me as well.

> The important part is actually returning and not throwing the error from `fetch`, allowing us to throw or handle the error as the end user sees fit.

Do you have any thoughts on how we'd expose that error handling to users? Anything like what we did for Drupal State: https://project.pages.drupalcode.org/drupal_state/en/error-handling/ or something different?

coby.sher’s picture

Do you have any thoughts on how we'd expose that error handling to users? Anything like what we did for Drupal State: https://project.pages.drupalcode.org/drupal_state/en/error-handling/ or something different?

At the moment I figured we'd just throw the error and let the user handle it via try/catch. Meaning, when we call our fetch method internally, if the error exists we will throw it. So we will throw from getResource or getCollection but not fetch itself.

We could implement an onError hook. Should that be an enhancement or part of this ticket?

brianperry’s picture

> At the moment I figured we'd just throw the error and let the user handle it via try/catch. Meaning, when we call our fetch method internally, if the error exists we will throw it. So we will throw from getResource or getCollection but not fetch itself.

That makes sense to me.

> We could implement an onError hook. Should that be an enhancement or part of this ticket?

Given what you outlined, feels like an enhancement.

coby.sher’s picture

Status: Active » Needs review
brianperry’s picture

Status: Needs review » Needs work
coby.sher’s picture

Status: Needs work » Needs review
brianperry’s picture

Status: Needs review » Fixed

Merged to canary and marked as fixed. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.