Ky: Request not cancelled when timeout occurs

[fruitraccoon]

When a Ky request times out AND no explicit signal was passed in the options, the request is not being cancelled.

Reproduction stackblitz here - when the code runs it makes two requests to GET root. In Chrome's dev tools, the first request is shown to be cancelled, but the second is not.

Looking at the code, it appears the issue is with this line. The this._options.signal is only set if there's already a signal set on the options.

This initial PR that added the cancel functionality had this coded differently but it was changed in PR 180. It does not appear that the change in behaviour was intended?

I've worked around this issue for now by setting a (never used) signal in the default options for ky, but I thought it worth reporting.

Edit: Tested Ky version: 0.16.1

[sholladay]

Yes, you are correct. I messed this up during refactoring in PR #180.

Thank you for reporting this!

[fruitraccoon]

Not a problem!

Just an update about my work-around (in case anyone else tries it) - unfortunately if you have a signal set on the default options, and then try to set another signal for a specific call, you get an exception (cryptically, TypeError: this._options.signal.addEventListener is not a function).

This is because when you have two signals, when Ky calls its deepMerge to combine them it unsuccessfully attempts to merge the signals together and the result is that options.signal is set to an empty object.

Personally I don't think this is a big deal, because in real usage it doesn't make a lot of sense to set a signal in the default options anyway 🙂

[sholladay]

Agreed that it doesn't make much sense to use the signal option with ky.create() or ky.extend(). That's an interesting quirk, nonetheless. One that's probably worth fixing unless doing so is complicated. My expectation would be for Ky to only try to merge _plain_ objects, thus signal should get overridden rather than merged, given that it is a non-plain object.

We could copy the algorithm from is-plain-obj.

[fruitraccoon]

That approach makes sense to me, and it would make the default options behaviour more robust. Nice.