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

Add TypeScript definition #31

Merged
merged 7 commits into from
Aug 20, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
interface ClearablePromise<T> extends Promise<T> {
/**
* Clears the delay and settles the promise.
*
* @memberof ClearablePromise
Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was generated automatically. I'll remove it.

*/
clear(): void;
}

declare const delay: {
/**
* Create a promise which resolves after the specified `milliseconds`.
* Optionally pass a `value` to resolve.
*
* @param {number} milliseconds Milliseconds to delay the promise.
* @param {T} [value] Value to resolve in the returned promise.
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

* @returns {ClearablePromise<T>} a promise which resolves after the specified `milliseconds`
Copy link
Owner

Choose a reason for hiding this comment

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

a => A and end it in a .

*/
<T = never>(milliseconds: number, value?: T): ClearablePromise<T>;
Copy link
Contributor

Choose a reason for hiding this comment

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

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>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any reason to do that?

Copy link
Contributor

@SamVerschueren SamVerschueren Aug 20, 2018

Choose a reason for hiding this comment

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

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

screen shot 2018-08-20 at 12 57 15

screen shot 2018-08-20 at 12 57 21

If you only have one, it is shown like this
screen shot 2018-08-20 at 12 56 58

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I see your point, thanks.


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

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, should be removed.

* @param {*} [reason] Value to reject in the returned promise.
Copy link
Owner

Choose a reason for hiding this comment

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

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

* @returns {ClearablePromise<never>} a promise which rejects after the specified `milliseconds`.
Copy link
Owner

Choose a reason for hiding this comment

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

a => A

*/
// TODO: Allow providing reason type after https://github.com/Microsoft/TypeScript/issues/5413 will be resolved.
reject(milliseconds: number, reason?: any): ClearablePromise<never>;
};

export default delay;
6 changes: 4 additions & 2 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,7 @@ const createDelay = willResolve => (ms, value) => {
return delayPromise;
};

module.exports = createDelay(true);
module.exports.reject = createDelay(false);
const delay = createDelay(true);
delay.reject = createDelay(false);
module.exports = delay;
module.exports.default = delay;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wow such recursive

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
"test": "xo && ava"
},
"files": [
"index.js"
"index.js",
"index.d.ts"
],
"keywords": [
"promise",
Expand Down