fix: throw early if signal is aborted before request is triggered

[kadhirr]

ky does not seem to abort a request if the signal was aborted before the request was triggered (ref).

We fix this by calling the throwIfAborted() method on the original signal.

[kadhirr]

I tried to add a test for this scenario

test('throws AbortError when signal was aborted before request', async t => {
	const server = await createHttpTestServer();
	// eslint-disable-next-line @typescript-eslint/no-empty-function
	server.get('/', () => {});

	const abortController = new AbortController();
	const {signal} = abortController;
	const request = new Request(server.url, {signal});
	abortController.abort();
	const response = ky(request);

	const error = (await t.throwsAsync(response))!;


	t.true(['DOMException', 'Error'].includes(error.constructor.name), `Expected DOMException or Error, got ${error.constructor.name}`);
	t.is(error.name, 'AbortError', `Expected AbortError, got ${error.name}`);
});

here I am facing the following issue

 ✘ [fail]: throws AbortError when signal was aborted before request Rejected promise returned by test
  ─

  throws AbortError when signal was aborted before request

  test/main.ts:724

   723:   const request = new Request(server.url, {signal});
   724:   abortController.abort();
   725:   const response = ky(request);

  Rejected promise returned by test. Reason:

  DOMException {}

  › <anonymous> (test/main.ts:724:18)

  ─

  1 test failed

seems like the abort() line is rejecting the promise, but there is no object where the test will listen for a async throw. Can you guide me on how to proceed here?

[sholladay]

I think the problem is that the throwIfAborted() call is happening inside of the constructor, which is not async or promise based. So the ky() call in the test isn't returning a rejected promise, but rather throwing a traditional synchronous exception. That immediately stops the test from executing and there's nothing to catch it, because throwsAsync() only handles promise based errors.

Perhaps we should just copy the signal's state instead of using throwIfAborted(). So basically abort our signal if the original is already aborted and be sure to copy over the reason.

That way fetch() will throw for us. And since that part of the code is wrapped in an async function, the error will inherently be returned via a promise and thus your test should pass, probably without any changes.

[kadhirr]

@sholladay I tried to copy the state of the original signal, but all abortController properties are readonly, not sure how to copy the state here. So I've aborted the internal abortController if the passed in signal was also aborted. The tests also seem to work with this usage.
Please let me know if this is sufficient!

[sholladay]

Yep, that's what I meant by copying the state. Could you also pass in the abort reason? Same as what's done in the event listener immediately below what you've written.

[kadhirr]

Hey @sholladay , I've added the reason to the abort(), forgot that in the previous commit. Hope this looks good!

[sholladay]

One last thing if you don't mind. I think it would be a good idea to fail the test if the server receives any requests.

[kadhirr]

One last thing if you don't mind. I think it would be a good idea to fail the test if the server receives any requests.

No issues @sholladay! I have modified the test to check if the server receives a request as well. Let me know if this looks good.

[sholladay]

Great work, @kadhirr. Thanks!