Make timeout clearable #15

ilkerceng · 2020-06-07T19:14:06Z

Hi, i just tried to fix #6

sindresorhus · 2020-06-08T05:57:58Z

This will need some tests.

ilkerceng · 2020-06-08T14:52:45Z

I tried but couldn't mock clearTimeout global method, using sinon and fakeTimers to be able to test like with haveBeenCalled. I have searched ava main repo also and found this suggestion but couldn't apply it. And also tried to ninos by jamie, and also couldn't apply it. Do you have any suggestion to mock clearTimeout like jest timer mocks ?

ilkerceng · 2020-06-19T14:26:43Z

hi @sindresorhus, i am sorry for bothering you, is there anything wrong with the code that i can fix.

sindresorhus · 2020-07-15T18:21:07Z

This will need doc updates to the readme and updates to index.d.ts.

index.js

test.js

sindresorhus · 2020-07-15T18:19:34Z

test.js

@@ -55,3 +55,10 @@ test('calls `.cancel()` on promise when it exists', async t => {
 	await t.throwsAsync(pTimeout(promise, 50), pTimeout.TimeoutError);
 	t.true(promise.isCanceled);
 });
+
+test('clears timeout when clear triggered', t => {


This could be better written.

sindresorhus · 2020-07-15T18:20:00Z

index.js

-			} catch (error) {
-				reject(error);
-			}
+	const p = new Promise((resolve, reject) => {


Use descriptive variable names

sindresorhus · 2020-07-15T18:20:06Z

test.js

+
+test('clears timeout when clear triggered', t => {
+	const pT = pTimeout(delay(200), 50);
+	pT.clear();


Use descriptive variable names

sindresorhus · 2020-07-15T18:22:04Z

As for testing, you can just check the time the promise took to resolve. See: https://github.com/sindresorhus/p-map/blob/master/test.js

sindresorhus · 2020-07-15T18:22:35Z

The PR needs a better title. It succinctly should describe what's being implemented.

ilkerceng · 2020-08-05T22:28:12Z

hi @sindresorhus, I have tried my best to update the code with your specifications. But I didn't understand why test failed. It runs well on my machine windows 10.

sindresorhus · 2020-08-08T11:44:38Z

Try to do some debugging yourself. Do some console logging of what the value is for the failing test and then you can use that info to further look into why it's failing on Travis.

ilkerceng · 2020-08-08T12:37:10Z

Try to do some debugging yourself. Do some console logging of what the value is for the failing test and then you can use that info to further look into why it's failing on Travis.

I have tried to debug, and re-run the job from forked repository, but didn't get any error. here is the logs: https://travis-ci.com/github/ilkerceng/p-timeout/jobs/369961763

sindresorhus · 2020-08-10T12:18:48Z

You still need to update the readme and add a doc comment to the clear method in index.d.ts.

index.d.ts

sindresorhus · 2020-09-03T13:26:02Z

index.d.ts

@@ -40,7 +40,7 @@ declare const pTimeout: {
 	): ClearablePromise<ValueType>;

 	/**
-	Timeout a promise after a specified amount of time.
+	Timeout a promise after a specified amount of time. It returns a decorated promise having `.clear()` method to be able to clear timeout.


This is not in the same location as the readme. I think it could also be better written.

I refactored the docs. But really don't know how it could be better :)

You didn't fix it in both places.

This addition should be removed.

sindresorhus · 2020-09-05T21:43:17Z

index.d.ts


 	If you pass in a cancelable promise, specifically a promise with a `.cancel()` method, that method will be called when the `pTimeout` promise times out.

 	@param input - Promise to decorate.
 	@param milliseconds - Milliseconds before timing out.
 	@param message - Specify a custom error message or error. If you do a custom error, it's recommended to sub-class `pTimeout.TimeoutError`. Default: `'Promise timed out after 50 milliseconds'`.
-	@returns A decorated `input` that times out after `milliseconds` time.
+	@returns A decorated `input` that times out after `milliseconds` time. Decorated input has a `.clear()` method, that method can be used to clear timeout.


Suggested change

@returns A decorated `input` that times out after `milliseconds` time. Decorated input has a `.clear()` method, that method can be used to clear timeout.

@returns A decorated `input` that times out after `milliseconds` time. It has a `.clear()` method that clears the timeout.

sindresorhus · 2020-09-05T21:44:07Z

index.d.ts

@@ -7,6 +7,10 @@ declare namespace pTimeout {
 	type TimeoutError = TimeoutErrorClass;
 }

+interface ClearablePromise<T> extends Promise<T>{
+	clear: () => void


Needs a short doc-comment.

sindresorhus · 2020-09-24T13:52:56Z

index.d.ts

@@ -7,16 +7,23 @@ declare namespace pTimeout {
 	type TimeoutError = TimeoutErrorClass;
 }

+/**
+ * Promise with `.clear() method


This is not useful. It just describes what the types already clearly define. The doc comment should describe what it does and its purpose for humans.

It's also in the incorrect place.

Hi, what about Promise with .clear() method that clears the timeout.?
And i didn't understand why it is in the incorrect place. It is just on top of interface declaration.

The doc comment should be placed above the clear: () and should describe what **.clear() does.

sindresorhus · 2020-12-01T15:57:48Z

@ilkerceng Bump

…ction from outside

ilkerceng · 2020-12-05T19:29:38Z

@sindresorhus bump :)

ilkerceng · 2020-12-05T19:54:22Z

Hi @sindresorhus, it gives these errors but they are not in my committed codes. I think, existing codebase v4 gives these errors.

Error: /home/runner/work/p-timeout/p-timeout/index.test-d.ts: line 1, col 1, Error - AVA ignores this file. (ava/no-ignored-test-files)
Error: /home/runner/work/p-timeout/p-timeout/index.test-d.ts: line 35, col 46, Error - Functions that return promises must be async. (@typescript-eslint/promise-function-async)
Error: /home/runner/work/p-timeout/p-timeout/index.test-d.ts: line 41, col 47, Error - Functions that return promises must be async. (@typescript-eslint/promise-function-async)

ilkerceng · 2020-12-12T19:30:55Z

Hi @sindresorhus, i think it is ok now

ilkerceng · 2020-12-19T19:01:33Z

Do i miss any review request in my code? If any, i can fix it.

sindresorhus · 2020-12-20T06:40:39Z

I'm just a bit burned out by this PR. I've already spent more than 10x the time to review this than it would have taken me to implement it myself. In the future, try to spend more time on a pull request before submitting it.

sindresorhus · 2020-12-20T06:38:36Z

index.d.ts

@@ -93,14 +100,15 @@ declare const pTimeout: {
 	pTimeout(delayedPromise(), 50, () => {
 		return pTimeout(delayedPromise(), 300);
 	});
+	promise.clear(); //Clear timeout


This is not relevant to the current example, because it makes some of the code moot here. Please remove.

sindresorhus · 2020-12-20T06:39:04Z

index.d.ts

@@ -40,7 +40,7 @@ declare const pTimeout: {
 	): ClearablePromise<ValueType>;

 	/**
-	Timeout a promise after a specified amount of time.
+	Timeout a promise after a specified amount of time. It returns a decorated promise having `.clear()` method to be able to clear timeout.


This addition should be removed.

sindresorhus · 2020-12-20T06:39:37Z

index.d.ts

+	/**
+	 * .clear() method that clears the timeout.
+	 */


Suggested change

/**

* .clear() method that clears the timeout.

*/

/**

Clear the timeout.

*/

ilkerceng · 2020-12-20T10:04:51Z

I'm just a bit burned out by this PR. I've already spent more than 10x the time to review this than it would have taken me to implement it myself. In the future, try to spend more time on a pull request before submitting it.

You are absolutely right. I'm really sorry.

sindresorhus closed this Jul 15, 2020

sindresorhus reopened this Jul 15, 2020

sindresorhus requested changes Jul 15, 2020

View reviewed changes

ilkerceng changed the title ~~promise decorated with adding clear function to be able to call clearTimeout from outside~~ Make the resulting promise cancelable Aug 5, 2020

ilkerceng changed the title ~~Make the resulting promise cancelable~~ Make timeout clearable Aug 5, 2020

ilkerceng requested a review from sindresorhus September 1, 2020 20:29

sindresorhus reviewed Sep 3, 2020

View reviewed changes

index.d.ts Outdated Show resolved Hide resolved

sindresorhus reviewed Sep 3, 2020

View reviewed changes

ilkerceng requested a review from sindresorhus September 5, 2020 07:21

sindresorhus reviewed Sep 5, 2020

View reviewed changes

ilkerceng requested a review from sindresorhus September 11, 2020 16:29

sindresorhus reviewed Sep 24, 2020

View reviewed changes

ilkerceng added 6 commits December 5, 2020 22:10

promise decorated with adding clear function to call clearTimeout fun…

008e8a4

…ction from outside

added canceled field, and test

4cbbd72

Make the resulting promise cancelable

1a972dc

Make the resulting promise cancelable

1dae5b6

return clearable promise type

f6c2339

clearable promise return type

dcc85e4

ilkerceng and others added 6 commits December 5, 2020 22:20

update README and index.d.ts for docs

834cc63

refactor docs for clear method

f0be27b

updated docs

f381abb

update docs

1a6d375

update doc

673c934

Update index.d.ts

aaae109

ilkerceng force-pushed the master branch from 5226814 to aaae109 Compare December 5, 2020 19:22

semicolon added

3cdbdd9

ilkerceng requested a review from sindresorhus December 5, 2020 19:39

ilkerceng added 2 commits December 5, 2020 22:43

missing test

365e9aa

missing new line

4f6a011

ilkerceng added 3 commits December 12, 2020 22:22

Merge remote-tracking branch 'upstream/master'

fd6607a

rollback

d2fd3d0

test update

1211b40

sindresorhus requested changes Dec 20, 2020

View reviewed changes

update doc

3d47f35

sindresorhus added 2 commits December 26, 2020 17:40

Update index.d.ts

481eae5

Update test.js

3e2d86d

sindresorhus merged commit e26f08d into sindresorhus:master Dec 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make timeout clearable #15

Make timeout clearable #15

ilkerceng commented Jun 7, 2020

sindresorhus commented Jun 8, 2020

ilkerceng commented Jun 8, 2020

ilkerceng commented Jun 19, 2020

sindresorhus commented Jul 15, 2020

sindresorhus Jul 15, 2020

sindresorhus Jul 15, 2020

sindresorhus Jul 15, 2020

sindresorhus commented Jul 15, 2020

sindresorhus commented Jul 15, 2020

ilkerceng commented Aug 5, 2020

sindresorhus commented Aug 8, 2020

ilkerceng commented Aug 8, 2020

sindresorhus commented Aug 10, 2020

sindresorhus Sep 3, 2020

ilkerceng Sep 3, 2020

sindresorhus Sep 5, 2020

sindresorhus Dec 20, 2020

sindresorhus Sep 5, 2020

sindresorhus Sep 5, 2020 •

edited

sindresorhus Sep 24, 2020

sindresorhus Sep 24, 2020

ilkerceng Oct 4, 2020

sindresorhus Oct 4, 2020

sindresorhus commented Dec 1, 2020

ilkerceng commented Dec 5, 2020

ilkerceng commented Dec 5, 2020

ilkerceng commented Dec 12, 2020

ilkerceng commented Dec 19, 2020

sindresorhus commented Dec 20, 2020

sindresorhus Dec 20, 2020

sindresorhus Dec 20, 2020

sindresorhus Dec 20, 2020

ilkerceng commented Dec 20, 2020

	@returns A decorated `input` that times out after `milliseconds` time. Decorated input has a `.clear()` method, that method can be used to clear timeout.
	@returns A decorated `input` that times out after `milliseconds` time. It has a `.clear()` method that clears the timeout.

Make timeout clearable #15

Make timeout clearable #15

Conversation

ilkerceng commented Jun 7, 2020

sindresorhus commented Jun 8, 2020

ilkerceng commented Jun 8, 2020

ilkerceng commented Jun 19, 2020

sindresorhus commented Jul 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Jul 15, 2020

sindresorhus commented Jul 15, 2020

ilkerceng commented Aug 5, 2020

sindresorhus commented Aug 8, 2020

ilkerceng commented Aug 8, 2020

sindresorhus commented Aug 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus Sep 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Dec 1, 2020

ilkerceng commented Dec 5, 2020

ilkerceng commented Dec 5, 2020

ilkerceng commented Dec 12, 2020

ilkerceng commented Dec 19, 2020

sindresorhus commented Dec 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilkerceng commented Dec 20, 2020

sindresorhus Sep 5, 2020 •

edited