fix(fetch): use AbortError DOMException

[KhafraDev]

This commit uses the proper error when a request is aborted. A WPT actually checks for this behavior.

---

Honestly I didn't know if I should have put DOMException in constants or in util. I can change it if it matters or there's a preference.

[codecov-commenter]

Codecov Report

Merging #1511 (edc4f54) into main (7d9fd51) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1511      +/-   ##
==========================================
+ Coverage   94.63%   94.65%   +0.02%     
==========================================
  Files          50       50              
  Lines        4589     4588       -1     
==========================================
  Hits         4343     4343              
+ Misses        246      245       -1     

Impacted Files	Coverage Δ
lib/core/errors.js	100.00% <ø> (ø)
lib/fetch/response.js	94.21% <ø> (-0.04%)	⬇️
lib/fetch/constants.js	100.00% <100.00%> (ø)
lib/fetch/index.js	81.15% <100.00%> (-0.04%)	⬇️
lib/client.js	97.76% <0.00%> (+0.12%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d9fd51...edc4f54. Read the comment docs.

[mcollina]

We should have a DomException in Node, no need for polyfill.

[KhafraDev]

Somehow missed this notification 😕.

DOMException is available in node globally from v17 and above, but fetch supports v16.5 and above. There's a comment detailing why this is needed a few lines below.

[mcollina]

You can get it with:

try {
  atob('~')
} catch (err) {
  const DOMException = Object.getPrototypeOf(err).constructor

  console.log(new DOMException('aaa'))
}

[KhafraDev]

Done in edc4f54

[LiviaMedeiros]

Should this support user-provided reason and TimeoutError?

[mcollina]

Clearly marking that changes should br made here

[KhafraDev]

Should this support user-provided reason and TimeoutError?

No, the spec requires an AbortError DOMException and the error is verbatim from Firefox.

Clearly marking that changes should br made here

I'm unsure what you mean by this

[LiviaMedeiros]

Clearly marking that changes should br made here

If you meant my question, it's completely not blocking. :)
Even if we decide that changes must be made, I believe it can be done in a follow-up PR instead.

No, the spec requires an AbortError DOMException and the error is verbatim from Firefox.

Do we have any way to distinguish between arbitrary AbortError and TimeoutError? If not, I'm confused with how should AbortSignal.timeout() work in userland.

AFAIK there were¹² common reasons in Node.js to use AbortError so if (err.name === 'AbortError') keeps being robust, but we definitely need a way to have more information. Is there a workaround?

This feature is not widely implemented yet, and timeout was added in Firefox only in v100 so its current behaviour doesn't convince me yet.

Footnotes

1. https://github.com/nodejs/node/issues/36084 ↩

2. https://github.com/nodejs/node/issues/40692 ↩

[KhafraDev]

Do we have any way to distinguish between arbitrary AbortError and TimeoutError? If not, I'm confused with how should AbortSignal.timeout() work in userland.

Yes, you can use the .name or .code property. However, until the spec is updated, an AbortError DOMException should still be thrown.

const abortError = new DOMException('The operation was aborted.', 'AbortError')

abortError.name // 'AbortError'

abortError.code === DOMException.ABORT_ERR

There doesn't seem to be any discussion in the fetch spec repo, this should probably be followed up there. However both Chrome & Firefox throw AbortError DOMExceptions.

AFAIK there were12 common reasons in Node.js to use AbortError so if (err.name === 'AbortError') keeps being robust, but we definitely need a way to have more information. Is there a workaround?

In the issue mentioned, Node.js does throw DOMExceptions for DOM APIs.. You can still use error.name === 'AbortError'.

[LiviaMedeiros]

1. Providing custom reason to AbortController.abort():

const controller = new AbortController();
setTimeout(() => controller.abort('custom reason'), 10);

fetch('https://github.com/nodejs/undici/pull/1511', {
  signal: controller.signal
}).catch(err => {
  console.error(err.name, err.code, err.message, err.cause);
});

2. Using AbortSignal.timeout():

fetch('https://github.com/nodejs/undici/pull/1511', {
  signal: AbortSignal.timeout(10)
}).catch(err => {
  console.error(err.name, err.code, err.message, err.cause);
});

In both cases I'm getting generic AbortError ABORT_ERR The operation was aborted undefined.

As an user, in (1), I need to have a way to retrieve custom reason.
In (2), I need to have a way to retrieve TimeoutError (or Node.js-specific code with same meaning).

Is it retrievable without having an explicit reference to signal.reason? Should this be addressed in undici, or Node.js, or DOM specs?

// This acknowledges TimeoutError in Node.js and Firefox >=100; Chrome <=102 doesn't support it
AbortSignal.timeout(10).addEventListener('abort', function(err) { console.info(this.reason.name) }); // TimeoutError

[KhafraDev]

I'm getting the same results in the browser, this should be suggested at https://github.com/whatwg/fetch

[LiviaMedeiros]

Thanks, I'll go ahead and fill an issue to WHATWG.

Some references from brief searching:

• There is a seemingly related opened PR for fetch specs: Add abort reason to abort fetch whatwg/fetch#1343
• PR which added AbortSignal.timeout() strongly emphasizes on ability to distinguish errors: Introduce AbortSignal.timeout() whatwg/dom#1032

My current impression (and suggestion to consider for undici implementation) on this is that having error.cause == signal.reason would be the best solution. But I might be wrong. :)

[mcollina]

lgtm