Add TypeScript definition #31

dmitry-korolev · 2018-08-09T12:09:18Z

No description provided.

sindresorhus · 2018-08-20T05:41:07Z

index.d.ts

+
+declare const delay: {
+  <T = never>(ms: number, value?: T): ClearablePromise<T>
+  reject(ms: number, reason?: any): ClearablePromise<never>


Why is this not also T?

Typescript Promise definition doesn't allow to set an error type. Since the Promise, created by reject will always fail, it should have never type.

I found the relevant TS issue: microsoft/TypeScript#5413 Can you add a TODO comment about making it more strict when that issue is fixed?

sindresorhus · 2018-08-20T05:41:47Z

index.d.ts

@@ -0,0 +1,10 @@
+interface ClearablePromise<T> extends Promise<T> {
+  clear (): void


clear(): void

sindresorhus · 2018-08-20T05:42:40Z

index.d.ts

+}
+
+declare const delay: {
+  <T = never>(ms: number, value?: T): ClearablePromise<T>


I know it's ms in the docs, but can you use milliseconds here for clarity?

sindresorhus · 2018-08-20T05:46:17Z

package.json

@@ -18,6 +18,7 @@
 	"files": [
 		"index.js"
 	],
+	"typings": "index.d.ts",


There's is no need to add this since it's named index, but you do need to add it to the files property.

sindresorhus · 2018-08-20T05:46:36Z

index.d.ts

+  reject(ms: number, reason?: any): ClearablePromise<never>
+}
+
+export = delay


Can you use export default delay;?

export default isn't going to work because it's just CJS. So if you would write module.exports.default = delay, we could use export default delay; here as well.

Yeah, let's do that then. I think it's much nicer to allow TS consumer to use ES2015 import syntax.

sindresorhus · 2018-08-20T06:35:45Z

@dmitry-korolev @SamVerschueren Any feedback on https://github.com/sindresorhus/typescript-definition-style-guide ? Is it clear enough? Anything missing?

sindresorhus · 2018-08-20T07:28:31Z

Could you add docs to the methods? You can just take the text from the readme.

SamVerschueren · 2018-08-20T07:32:00Z

The style guide looks good! Let's work out the testing first and then we could add it later as well, would be nice.

dmitry-korolev · 2018-08-20T08:06:08Z

index.js

+const delay = createDelay(true);
+delay.reject = createDelay(false);
+module.exports = delay;
+module.exports.default = delay;


wow such recursive

sindresorhus · 2018-08-20T08:11:43Z

index.d.ts

+	 * Optionally pass a `reason` to reject.
+	 *
+	 * @param {number} milliseconds Milliseconds to delay the promise.
+	 * @param {*} [reason] Value to reject in the returned promise.


I assume * is a shortcut for any, but isn't it better to stay consistent with the actual typing and use any?

sindresorhus · 2018-08-20T08:11:59Z

index.d.ts

+	/**
+	 * Clears the delay and settles the promise.
+	 *
+	 * @memberof ClearablePromise


I'm not that familiar with TS documentation, but isn't this moot as it's already clear from the context and TS should be able to infer it itself?

This was generated automatically. I'll remove it.

sindresorhus · 2018-08-20T08:12:47Z

index.d.ts

+	 * Create a promise which rejects after the specified `milliseconds`.
+	 * Optionally pass a `reason` to reject.
+	 *
+	 * @param {number} milliseconds Milliseconds to delay the promise.


Is it required to provide the types here? I feel it's moot as it's already defined in the signature and TS could just get it from there. I don't like needless boilerplate.

Seems it's not: https://github.com/Microsoft/tsdoc

According to TSDoc docs, there should be - between the param name and description.

Yes, should be removed.

sindresorhus · 2018-08-20T08:14:04Z

index.d.ts

+	 *
+	 * @param {number} milliseconds Milliseconds to delay the promise.
+	 * @param {*} [reason] Value to reject in the returned promise.
+	 * @returns {ClearablePromise<never>} a promise which rejects after the specified `milliseconds`.


sindresorhus · 2018-08-20T08:16:48Z

index.d.ts

+	 *
+	 * @param {number} milliseconds Milliseconds to delay the promise.
+	 * @param {T} [value] Value to resolve in the returned promise.
+	 * @returns {ClearablePromise<T>} a promise which resolves after the specified `milliseconds`


a => A and end it in a .

sindresorhus · 2018-08-20T08:20:25Z

index.d.ts

+	 * Optionally pass a `value` to resolve.
+	 *
+	 * @param {number} milliseconds Milliseconds to delay the promise.
+	 * @param {T} [value] Value to resolve in the returned promise.


[value], is the [] really necessary? The signature already defines it as optional.

I think [] is a leftover from JSDoc, because there was no way of indicating optionals. But I think they should be removed for TS.

sindresorhus · 2018-08-20T08:20:55Z

Sorry for all the nitpick, but I intend to use this one as an example for other submissions, so I want to perfect it ;)

SamVerschueren · 2018-08-20T08:25:41Z

index.d.ts

+	 * @param {T} [value] Value to resolve in the returned promise.
+	 * @returns {ClearablePromise<T>} a promise which resolves after the specified `milliseconds`
+	 */
+	<T = never>(milliseconds: number, value?: T): ClearablePromise<T>;


I think you can write the 2 methods separately. This way you have 2 overrides.

(milliseconds: number): ClearablePromise<void>; <T = any>(milliseconds: number, value: T): ClearablePromise<T>

Is there any reason to do that?

If you use the two lines, it clearly shows that it has 2 overloads. So you can also show other descriptions per method.

If you only have one, it is shown like this

(Docs are coming from @types/delay, so don't bother naming :))

Sorry, I still don't get the difference.
As for me, the suggested overload is just another way to make the value argument optional. The return types would still be the same. Descriptions also will be identical, since it has to be explicitly stated that the method accepts the second argument.
Finally, the suggested overload won't affect intellisense or type inference in any way.

As for me, the suggested overload is just another way to make the value argument optional.

It is just another way of making it optional.

The return types would still be the same.

The one is returning ClearablePromise<void>, the other is returning ClearablePromise<T>. With the current implementation, you can write the following line of code.

const result = await delay<string>(2000);

And TypeScript will think that result is of type string, which it clearly isn't because there is no value provided.

When using overloads, you won't be able to write that line because it isn't accepted by the compiler.

Now I see your point, thanks.

SamVerschueren · 2018-08-20T08:26:14Z

index.d.ts

+	 * Create a promise which rejects after the specified `milliseconds`.
+	 * Optionally pass a `reason` to reject.
+	 *
+	 * @param {number} milliseconds Milliseconds to delay the promise.


Yes, should be removed.

sindresorhus · 2018-08-20T17:51:48Z

This is looking great now 🙌 Thanks for your patience @dmitry-korolev :)

Add typescript typings

5d9f202

sindresorhus reviewed Aug 20, 2018

View reviewed changes

index.d.ts Outdated

@@ -0,0 +1,10 @@

interface ClearablePromise<T> extends Promise<T> {

clear (): void

Copy link

Owner

sindresorhus Aug 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clear(): void

sindresorhus reviewed Aug 20, 2018

View reviewed changes

sindresorhus changed the title ~~Add typescript typings~~ Add TypeScript typings Aug 20, 2018

sindresorhus changed the title ~~Add TypeScript typings~~ Add TypeScript definition Aug 20, 2018

dmitry-korolev added 3 commits August 20, 2018 10:41

Add tsdoc comments and default export

d01ed3a

Fix code style

d5a7a44

Add todo notice about reason type

0204f6e

dmitry-korolev commented Aug 20, 2018

View reviewed changes

sindresorhus reviewed Aug 20, 2018

View reviewed changes

SamVerschueren suggested changes Aug 20, 2018

View reviewed changes

dmitry-korolev and others added 3 commits August 20, 2018 11:26

Remove type definitions from tsdoc comments

b5f8fd2

Split delay type definition into two overloads

1a17bee

Update index.d.ts

4a197ec

sindresorhus merged commit 3e383c6 into sindresorhus:master Aug 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TypeScript definition #31

Add TypeScript definition #31

dmitry-korolev commented Aug 9, 2018

sindresorhus Aug 20, 2018

dmitry-korolev Aug 20, 2018

sindresorhus Aug 20, 2018

sindresorhus Aug 20, 2018

sindresorhus Aug 20, 2018

sindresorhus Aug 20, 2018

sindresorhus Aug 20, 2018

sindresorhus Aug 20, 2018

SamVerschueren Aug 20, 2018

sindresorhus Aug 20, 2018

sindresorhus commented Aug 20, 2018

sindresorhus commented Aug 20, 2018

SamVerschueren commented Aug 20, 2018

dmitry-korolev Aug 20, 2018

sindresorhus Aug 20, 2018

sindresorhus Aug 20, 2018

dmitry-korolev Aug 20, 2018

sindresorhus Aug 20, 2018

sindresorhus Aug 20, 2018

sindresorhus Aug 20, 2018

SamVerschueren Aug 20, 2018

sindresorhus Aug 20, 2018

sindresorhus Aug 20, 2018

sindresorhus Aug 20, 2018

SamVerschueren Aug 20, 2018

sindresorhus commented Aug 20, 2018

SamVerschueren Aug 20, 2018

dmitry-korolev Aug 20, 2018

SamVerschueren Aug 20, 2018 •

edited

Loading

dmitry-korolev Aug 20, 2018

SamVerschueren Aug 20, 2018

dmitry-korolev Aug 20, 2018

SamVerschueren Aug 20, 2018

sindresorhus commented Aug 20, 2018

		@@ -0,0 +1,10 @@
		interface ClearablePromise<T> extends Promise<T> {
		clear (): void

Add TypeScript definition #31

Add TypeScript definition #31

Conversation

dmitry-korolev commented Aug 9, 2018

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

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 Aug 20, 2018

sindresorhus commented Aug 20, 2018

SamVerschueren commented Aug 20, 2018

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamVerschueren Aug 20, 2018 • edited Loading

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 Aug 20, 2018

SamVerschueren Aug 20, 2018 •

edited

Loading