Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support AbortSignal #37

Open
wants to merge 3 commits into
base: next
Choose a base branch
from
Open

Conversation

kanongil
Copy link
Contributor

This is done in 2 ways:

  1. Add a signal option to rethrow() and ignore().
  2. Add extra functions to identify AbortSignal derived aborts and timeouts.

Regarding 1, this allows a Bounce user to simply hook a signal.rethrowIfAborted() into the call. Eg.:

function doStuff(options) {

    const signal = options.signal;
    try {
        const res = await fetch(options.url, { signal });
    }
    catch (err) {
        Bounce.rethrow(err, 'system', { signal });

        // handle non-system, non-signalled error, maybe with a retry
    }
}

Alternatively, you would need an extra signal.rethrowIfAborted() before the call to Bounce, which gets verbose:

signal.rethrowIfAborted();
Bounce.rethrow(err, 'system');

Regarding 2, the default errors thrown when calling abort() or timeout() have no shared prototype and have changed in various node releases. As such you cannot do an instanceof check with eg. Bounce.rethrow(err, AbortError). The most compatible way to detect this, is to simply check that the name property is AbortError. Given this, I have added shortcuts to do it safely.

Note that I have marked these as breaking, not because of the API changes, but solely because it requires more modern versions of node (v16.17+) then the current release.

@kanongil kanongil added feature New functionality or improvement breaking changes Change that can breaking existing code labels Nov 30, 2023
Copy link
Contributor

@Marsup Marsup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! I think the use of DOMException even requires node 17+, so that's a good fit for our next iteration. Do you foresee use of that in hapi itself?

@devinivy
Copy link
Member

I'd say yes—I hope and expect we are able to incorporate abort signals into hapi itself as well!

@Marsup
Copy link
Contributor

Marsup commented Oct 23, 2024

I'll fix the CI.

@Marsup
Copy link
Contributor

Marsup commented Oct 23, 2024

You can now rebase both your PRs, they should pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes Change that can breaking existing code feature New functionality or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants