feat: special handling for AbortError

[mcataford]

Description

This aims to address #362 and adds specific handling for AbortError being thrown during a request. In the present state of things, if a user were to pass an AbortSignal via options.request.signal, the resulting error gets lumped in with a generic RequestError and the fact that the request was aborted is obscured.

With the present changes, the AbortError is bubbled up (similarly to how RequestError instances thrown by fetch are) so that the user can handle it however they want.

I'm more than open to input if you have a better vision for what should be included in the error that's rethrown (i.e. if it should be wrapped the same way the catchall RequestError is) if simply bubbling up the AbortError isn't enough. Since request cancellation is intentional, I would expect that as long as whoever calls request(...) can know and handle it properly, not much more is needed.

This also tosses in a sample test case to cover this behaviour.

---

View rendered README.md

[mcataford]

Since AbortController is fairly recent on Node, the test I added will need a bit more love to be Node <15 compatible.

Update: after toying a bit with it, there's a couple of options on the table --

The test relevant to the handling of request cancellation could be skipped for Node versions that don't support AbortController at all via something like

(typeof AbortController !== 'undefined' ? it : it.skip)('test description', () => { ... })

in which case the cancellation test would essentially only run in the Node v16 case of the matrix;

It's also possible to come up with a proper mock that would emulate the behaviour of AbortController / AbortSignal here so that we don't need the real thing in the first place (or so that we can sub it when Node doesn't offer it).

The latter option allows to preserve the test coverage rating in all environments, otherwise the AbortController-specific sections would be untested in Node 10/12.

I'm going to dig around a bit more into what that mock would look like.

[mcataford]

I ended up coming up with a mock that isn't terrible, although there's definitely room for improvement. It does allow the assertion that the error that is thrown via the AbortSignal is what is bubbled up, but because it's a mock, any further assertion would be shoddy.

An interesting option here would be to introduce some sort of AbortedRequestError that would be defined in octokit/request.js and that would cover the cancellation case, allowing us to insert custom details before we throw it. I'm going to mull that over and explore that route so the error thrown via request cancellation has as much details to it than any other error thrown.

Input welcome on what is preferred if applicable!

[wolfy1339]

Hi @mcataford !

We recently dropped support for any version for NodeJS < 14

Are you still interested in updating this and getting this completed

[mcataford]

Hi @mcataford !

We recently dropped support for any version for NodeJS < 14

Are you still interested in updating this and getting this completed

Absolutely! I'll take a look today and get this in order! 🚀

[mcataford]

Hi @mcataford !

We recently dropped support for any version for NodeJS < 14

Are you still interested in updating this and getting this completed

In principle, there's no update needed here since AbortController is still not supported by all versions of node supported by request.js (i.e. Node ^14 will need the test with mock). Do let me know if you have any more changes in mind.

Otherwise, I'll tidy up the PR and rebase the branch so it's clean.

✔️ Rebased so the branch is up to date;
✔️ Tidied up the branch history

[mcataford]

@wolfy1339 Noticed that this was out-of-date v. master, rebased again. 👍

Thank you for the review and follow-up! 🎉 Looking forward to seeing this merged! 🚀

[github-actions]

🎉 This PR is included in version 6.2.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀