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

Actually cancel the request after the timeout #27

Merged
merged 14 commits into from Feb 4, 2019

Conversation

Projects
None yet
8 participants
@ashinzekene
Copy link
Contributor

commented Sep 10, 2018

Fixes #8

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Sep 10, 2018

Awesome! Can you add a test?

@sindresorhus sindresorhus changed the title cancel request after timeout Actually cancel the request after the timeout Sep 10, 2018

index.js Outdated
@@ -99,6 +105,10 @@ class Ky {
...otherOptions
};

if (supportsCancel) {
this._options.signal = abortCtrl.abort();

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus Sep 11, 2018

Owner

This would override the user's signal.

index.js Outdated
@@ -1,6 +1,7 @@
'use strict';

const isObject = value => value !== null && typeof value === 'object';
const supportsCancel = typeof AbortController === 'function';

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus Sep 11, 2018

Owner

supportsCancel => supportsAbortController

index.js Outdated
@@ -77,6 +78,11 @@ class TimeoutError extends Error {
}
}

let abortCtrl;

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus Sep 11, 2018

Owner

abortCtrl => abortController

@ashinzekene

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

I need help with writing the tests

index.js Outdated
@@ -99,6 +105,10 @@ class Ky {
...otherOptions
};

if (supportsAbortController && !this._options.signal) {

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus Sep 11, 2018

Owner

We still want timeout to work, even if the user sets their own signal.

This comment has been minimized.

Copy link
@ashinzekene

ashinzekene Sep 11, 2018

Author Contributor

ookay....

Show resolved Hide resolved index.js Outdated
@ashinzekene
Copy link
Contributor Author

left a comment

Linting is failing because of this line

There is no addEventListener method on type AbortSignal

ashinzekene added some commits Sep 12, 2018

@taylorjdawson

This comment has been minimized.

Copy link

commented Sep 17, 2018

How is this PR coming along, @ashinzekene anything I can do to help?

@ashinzekene

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2018

I'm waiting for a review. You can also look at helping with tests

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Sep 29, 2018

@felixfbecker You have experience with AbortController, is this the correct way to handle it?

@felixfbecker
Copy link

left a comment

The signal should probably also be passed to delay() in the retry logic

index.js Outdated
@@ -77,12 +79,21 @@ class TimeoutError extends Error {
}
}

let abortController;

This comment has been minimized.

Copy link
@felixfbecker

felixfbecker Sep 30, 2018

A global AbortController for all requests? That seems wrong. It should be one controller per request

index.js Outdated
const delay = ms => new Promise(resolve => setTimeout(resolve, ms));

const timeout = (promise, ms) => Promise.race([
promise,
(async () => {
await delay(ms);
if (!isAborted && supportsAbortController) {
abortController.abort();
isAborted = true;

This comment has been minimized.

Copy link
@felixfbecker

felixfbecker Sep 30, 2018

Isn't isAborted the same as abortController.signal.aborted?

index.js Outdated
@@ -99,6 +110,14 @@ class Ky {
...otherOptions
};

if (supportsAbortController) {
if (this._options.signal) {
this._options.signal.addEventListener('abort', () => !isAborted && abortController.abort());

This comment has been minimized.

Copy link
@felixfbecker

felixfbecker Sep 30, 2018

abort() is idempotent so the isAborted check is not needed.

@ashinzekene

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2018

I believe this fixes the issues right ? @felixfbecker

@felixfbecker
Copy link

left a comment

Would it be possible to add a test for this? For example, if you set the timeout 0 or 1ms then wait, you should be able to assert that the Promise rejects with a TimeoutError.
Related, since this touches the AbortSignal, there should probably be a test that asserts you can still pass an AbortSignal manually (i.e. Promise is rejected with AbortError if signal is aborted).

Show resolved Hide resolved index.js
@ashinzekene

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2018

@ashinzekene

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2018

I'm finding it hard to test. I initially had to installed browser-env to be able to use AbortController on the test file, but then this still return undefined

@felixfbecker

This comment has been minimized.

Copy link

commented Oct 12, 2018

You shouldn't need a whole browser env with DOM etc for this. https://www.npmjs.com/package/abort-controller should be enough.

@ianwalter

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

@felixfbecker I don't think node-fetch supports AbortController yet bitinn/node-fetch#437 and I doubt jsdom does either.

It sounds like support for testing in a real browser will land in ky soon: #34

@ashinzekene

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2018

Cool @ianwalter

I found out that the tests I wrote kept failing. Didn't know if the issue was from node-fetch or abort-controller polyfill.

Was about raising the issue here

index.js Outdated
promise,
(async () => {
await delay(ms);
if (supportsAbortController) {

This comment has been minimized.

Copy link
@szmarczak

szmarczak Oct 25, 2018

Collaborator

if (abortController) {

Show resolved Hide resolved index.js Outdated
@szmarczak

This comment has been minimized.

Copy link
Collaborator

commented Oct 25, 2018

LGTM, we need only to test it on browsers somehow...

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Oct 31, 2018

@ianwalter I've added browser testing, so you can use this now to implement the tests: f0b7502 :)

@sindresorhus sindresorhus force-pushed the sindresorhus:master branch from 383d754 to f107e7b Nov 1, 2018

index.js Outdated
if (supportsAbortController) {
abortController = new AbortController();
if (this._options.signal) {
this._options.signal.addEventListener('abort', () => abortController.abort());

This comment has been minimized.

Copy link
@felixfbecker

felixfbecker Nov 8, 2018

This listener would need to be removed once the request is completed (or errored, i.e. in a finally), otherwise the listener would leak and stay around forever even though the request cannot be cancelled anymore

index.js Outdated
let abortController;

if (supportsAbortController) {
abortController = new AbortController();

This comment has been minimized.

Copy link
@sholladay

sholladay Nov 9, 2018

Collaborator

abortController should be defined as a const, since it's never re-assigned.

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Dec 15, 2018

@ianwalter Still interested in finishing this? :)

@ashinzekene

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2018

I've pushed fixes for this. But I need help writing tests. Can someone assist?
@felixfbecker @szmarczak @ianwalter

@ianwalter

This comment has been minimized.

Copy link
Contributor

commented Dec 15, 2018

I tried writing a test for this but doesn't look like headless cypress run (electron 59) supports AbortController. Will play around with it some more.

Update: Not 100% sure but seems like we should just wait for Cypress v4 which will upgrade Electron to v3 which will use Chrome v66 which will add support for this. I read that you can't polyfill AbortController with native fetch so that's doesn't seem like an option. Other option is to use puppeteer/karmatic instead of Cypress.

@felixfbecker

This comment has been minimized.

Copy link

commented Dec 15, 2018

Yeah if you just assign AbortController on window fetch will not honor the signal. Do the test make actual HTTP requests though? What if fetch is also polyfilled (the polyfill can use xhr under the hood which is cancellable)?

@ashinzekene

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2018

When I tried writing tests for it, I found out that it actually works.
I wrote this

	it('throws abort error when aborted by user', async () => {
		const abortController = new AbortController();
		const {signal} = abortController;
		const response = ky('https://jsonplaceholder.typicode.com/todos', {signal});
		expect(() => abortController.abort()).to.throw(window.AbortError);
		await response;
	});

this from cypress
image

The test fails but the request is actually aborted and I can't find a way to catch the error

However, if i write the test this way

	it('throws abort error when aborted by user', async () => {
		const abortController = new AbortController();
		const {signal} = abortController;
		const response = ky('https://jsonplaceholder.typicode.com/todos', {signal});
		expect(abortController.abort).to.throw(window.AbortError);
		await response;
	});

It passes but the request is not aborted
image

@ianwalter

This comment has been minimized.

Copy link
Contributor

commented Dec 15, 2018

I would have had a couple of working tests thanks to @felixfbecker's suggestion but forgot that using fetch (even if it's a polyfill) doesn't work with cy.route stubbing (cypress-io/cypress#95) so I have to find another way to handle the request and assert it's aborted.

@sholladay

This comment has been minimized.

Copy link
Collaborator

commented Dec 15, 2018

@felixfbecker

This comment has been minimized.

Copy link

commented Dec 16, 2018

Since when does window.AbortError exist?

@ashinzekene

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2018

Sorry, there's actually nothing like that. I got the error from Cypress. I think its a type of DOMException

@felixfbecker

This comment has been minimized.

Copy link

commented Dec 16, 2018

error.name will be AbortError

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Jan 17, 2019

Bump :)

@ianwalter ianwalter referenced this pull request Jan 20, 2019

Merged

Replace Cypress with AVA + Puppeteer #84

2 of 2 tasks complete

szmarczak added some commits Feb 2, 2019

@szmarczak szmarczak requested review from sindresorhus and sholladay Feb 2, 2019

@sindresorhus sindresorhus merged commit 8f17a13 into sindresorhus:master Feb 4, 2019

4 checks passed

Travis CI - Pull Request Build Passed
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to d5d3eaa
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Feb 4, 2019

Thank you, @ashinzekene :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.