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

[async] add Promise return values #37605

Closed
wants to merge 2 commits into from

Conversation

yume-chan
Copy link
Contributor

@yume-chan yume-chan commented Aug 14, 2019

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: feat: await-able Async methods caolan/async#1572
  • If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

To reviewers:

I have several questions when updating this definition:

AsyncIterableIterator:

The IterableCollection<T> type can also be AsyncIterableIterator<T>, but this type is in ES2018 and I don't know if we can use it here.

doWhilst:

The iteratee parameter will be called with (callback: (err: any, ...results: any[]) => void), and the test parameter will be called with (...results: any[], callback: (err: any, truth: boolean) => void), passing the results got from iteratee.

However, without type recursion, it's impossible to correctly typing both iteratee and test parameters. So I use Function for test here.

race:

The result of each task in tasks does not necessarily have the same type.

However, the compiler is not able to infer the result type (should be a union type) T from the tasks parameters, the user must specify it manually.

Also, the official API documentation is missing Promise support for this function.

type alias:

Some function definitions are aliases to others. For example eachSeries is an alias to each. But they just have the same signature by luck but they work differently. Shall we use aliases for different things but with the same signature?

series:

Functions like series can accept both Array and Object, but because the return value will also be Array or Object respectively, we can't use the universal IterableCollection<T> here.

Currently, the Array overload on these functions contains Array only. Shall we split IterableCollection<T> to Array and Object (Object already have Dictionary<T>)? What name is suited?

waterfall:

Functions like waterfall, seq and compose takes multiple functions, and pass the result from one to the next.

There is no way to correctly typing the relation between these parameter functions, and the type of the return value, right?

Also, the official API documentation is missing Promise support for this function.

AsyncFunction:

With AsyncFunction become a union type (can be either callback style or Promise style), the compiler can't infer the generic type T any more.
I believe we should change this, but how?

Testing:

I bought a test case from caolan/async here, added some type declarations, fixed some issues, so I can pass the typing test with TypeScript 3.0+.

But with the usage of argument spreading and other features introduced in TypeScript 3.0, this file won't pass compilation with TypeScript 2.3~2.9.

Currently I excluded this file from tsconfig.json. Is there a way to fix it (without upgrade type definition to use TypeScript 3.0)?

EDIT: The CI doesn't like unused files, so I removed it. You can review it at https://github.com/DefinitelyTyped/DefinitelyTyped/blob/21fe63ca69081b6a2d775bd7928587a727f191f9/types/async/test/awaitable.ts

@yume-chan yume-chan changed the title feat(async): add Promise return values [async] add Promise return values Aug 14, 2019
@yume-chan
Copy link
Contributor Author

Related #37325

@typescript-bot typescript-bot added this to Waiting for Reviewers in Pull Request Status Board Aug 14, 2019
@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Aug 14, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Aug 14, 2019

@yume-chan Thank you for submitting this PR!

🔔 @borisyankov @Kern0 @Penryn @fenying @pascalmartin @Dmitri1337 @erossignon @Juliiii - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot
Copy link
Contributor

👋 Hi there! I’ve run some quick performance metrics against master and your PR. This is still an experiment, so don’t panic if I say something crazy! I’m still learning how to interpret these metrics.

Let’s review the numbers, shall we?

Comparison details 📊
master #37605 diff
Batch compilation
Memory usage (MiB) 71.7 73.9 +3.1%
Type count 11246 11584 +3.0%
Assignability cache size 3572 3317 -7.1%
Subtype cache size 196 468 +138.8%
Identity cache size 41 37 -9.8%
Language service
Samples taken 1771 1795 +1.4%
Identifiers in tests 1771 1795 +1.4%
getCompletionsAtPosition
    Mean duration (ms) 365.9 363.7 -0.6%
    Median duration (ms) 363.9 362.4 -0.4%
    Mean CV 13.8% 12.3% -10.8%
    Worst duration (ms) 469.6 484.5 +3.2%
    Worst identifier email_link console
getQuickInfoAtPosition
    Mean duration (ms) 365.3 380.7 +4.2%
    Median duration (ms) 363.3 378.2 +4.1%
    Mean CV 13.7% 14.5% +5.9%
    Worst duration (ms) 477.5 502.1 +5.2%
    Worst identifier write_file make_folder
System information
Node version v10.16.0 v10.16.1
CPU count 2 2
CPU speed 2.294 GHz 2.294 GHz
CPU model Intel(R) Xeon(R) CPU E5-2673 v4 @ 2.30GHz Intel(R) Xeon(R) CPU E5-2673 v4 @ 2.30GHz
CPU Architecture x64 x64
Memory 6.8 GiB 6.8 GiB
Platform linux linux
Release 4.15.0-1051-azure 4.15.0-1052-azure

First off, note that the system varied slightly between these two runs, so you’ll have to take these measurements with a grain of salt.

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.


If you have any questions or comments about me, you can ping @andrewbranch. Have a nice day!

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Review in Pull Request Status Board Aug 19, 2019
@typescript-bot typescript-bot added the Unmerged The author did not merge the PR when it was ready. label Aug 19, 2019
@typescript-bot
Copy link
Contributor

After 5 days, no one has reviewed the PR 😞. A maintainer will be reviewing the PR in the next few days and will either merge it or request revisions. Thank you for your patience!

@sheetalkamat
Copy link
Contributor

This PR has questions in it. Popular package definitely needs to be reviewed by definition owners

@rbuckton
Copy link
Collaborator

rbuckton commented Sep 4, 2019

@yume-chan: If you want to leverage newer TypeScript features but maintain compatibility with older versions of TypeScript, we have some guidance here: https://github.com/DefinitelyTyped/DefinitelyTyped#i-want-to-use-features-from-typescript-31-or-above

Copy link
Contributor

@uniqueiniquity uniqueiniquity left a comment

Choose a reason for hiding this comment

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

Marking as changes requested until Ron's comment is addressed (so that the bot can handle this appropriately).

@typescript-bot typescript-bot moved this from Review to Needs Author Attention in Pull Request Status Board Sep 24, 2019
@typescript-bot typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed Awaiting reviewer feedback Unmerged The author did not merge the PR when it was ready. labels Sep 24, 2019
@typescript-bot
Copy link
Contributor

@yume-chan One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

@yume-chan
Copy link
Contributor Author

@rbuckton I have already read that, but the definition itself doesn't use TypeScript 3.0+ features, I just want to test new features against the definition file.

@rbuckton
Copy link
Collaborator

My comment was in response to the question in the PR description, not the changes in the PR itself. I concur with @sheetalkamat that we need a definition owner to weigh in on these changes.

@uniqueiniquity uniqueiniquity dismissed their stale review September 25, 2019 16:15

Dismissing review to re-tag definition owners.

@uniqueiniquity
Copy link
Contributor

Re-tagging @borisyankov @Kern0 @Penryn @fenying @pascalmartin @Dmitri1337 @erossignon @Juliiii for review of these changes - we don't feel that it's a good idea to merge without your input.

@levyitay
Copy link
Contributor

Is this being maintained?

@Robula
Copy link

Robula commented Mar 18, 2020

What's the hold-up on this? Can this be merged and we can worry about using newer Typescript features later?

@alvis
Copy link
Contributor

alvis commented Apr 2, 2020

Retagging @borisyankov @Kern0 @Penryn @fenying @pascalmartin @Dmitri1337 @erossignon @Juliiii.

I just have a review on the changes. As an async user, I can confirm that the changes look good to me. In fact, besides adding the missing promise types, the author also fixes some wrong return types such as unsaturated in AsyncQueue which has been changed to a Promise since 3.0. (see http://caolan.github.io/async/v3/docs.html#QueueObject)

As this PR has been held up for half a year already, and async 3 has already been released for almost a year. It should be time to move on and merge.

@typescript-bot typescript-bot added Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. and removed Revision needed This PR needs code changes before it can be merged. labels May 12, 2020
@typescript-bot
Copy link
Contributor

@yume-chan Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 - keep an eye on this comment as I'll be updating it with information as things progress.

Code Reviews

Because this PR edits the configuration file, it can be merged once it's reviewed by a DT maintainer.

Status

  • ❌ No merge conflicts
  • ✅ Continuous integration tests have passed
  • ❌ A DT maintainer needs to merge changes which affect module config files

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 37605,
  "author": "yume-chan",
  "owners": [
    "borisyankov",
    "kern0",
    "Penryn",
    "fenying",
    "pascalmartin",
    "Dmitri1337",
    "erossignon",
    "Juliiii",
    "brendtumi"
  ],
  "dangerLevel": "ScopedAndConfiguration",
  "headCommitAbbrOid": "2f79d19",
  "headCommitOid": "2f79d19f81374daff603d8bc65a8d505b3e56e8d",
  "mergeIsRequested": false,
  "stalenessInDays": 271,
  "lastCommitDate": "2019-08-14T04:54:56.000Z",
  "lastCommentDate": "2020-04-02T16:52:27.000Z",
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/37605/files",
  "hasMergeConflict": true,
  "authorIsOwner": false,
  "isFirstContribution": true,
  "popularityLevel": "Popular",
  "anyPackageIsNew": false,
  "packages": [
    "async"
  ],
  "files": [
    {
      "filePath": "types/async/index.d.ts",
      "kind": "definition",
      "package": "async"
    },
    {
      "filePath": "types/async/test/index.ts",
      "kind": "test",
      "package": "async"
    },
    {
      "filePath": "types/async/tsconfig.json",
      "kind": "package-meta",
      "package": "async"
    }
  ],
  "hasDismissedReview": true,
  "travisResult": "pass",
  "lastReviewDate": "2019-09-24T22:49:55.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 0,
  "isChangesRequested": false
}

@typescript-bot
Copy link
Contributor

🔔 @borisyankov @Kern0 @Penryn @fenying @pascalmartin @Dmitri1337 @erossignon @Juliiii @brendtumi - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

@typescript-bot
Copy link
Contributor

@yume-chan Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day!

@typescript-bot
Copy link
Contributor

@yume-chan To keep things tidy, we have to close PRs that aren't mergeable but don't have activity from their author. No worries, though - please open a new PR if you'd like to continue with this change. Thank you!

@elibarzilay elibarzilay removed this from Needs Author Attention in Pull Request Status Board Jul 13, 2020
@yume-chan yume-chan deleted the async branch March 21, 2023 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. Popular package This PR affects a popular package (as counted by NPM download counts). Question Unmergeable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants