[Bluebird] support then -> static .all -> then by patricklx · Pull Request #46720 · DefinitelyTyped/DefinitelyTyped

patricklx · 2020-08-13T15:51:01Z

e.g. Promise.resolve([x1, x2]).all().then([x1, x2])

typescript-bot · 2020-08-13T15:52:03Z

@patricklx Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

bluebird on npm, on unpkg

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

Status

✅ No merge conflicts
❌ Continuous integration tests have passed
❌ Most recent commit is approved by type definition owners or DT maintainers

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

Inactive

This PR has been inactive for 31 days — it is considered abandoned!

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 46720,
  "author": "patricklx",
  "owners": [
    "lhecker"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "bc57437",
  "headCommitOid": "bc574372c5fd103179e12c3e1ccfcdcff5e37fb1",
  "mergeIsRequested": false,
  "stalenessInDays": 31,
  "lastPushDate": "2020-08-20T12:39:37.000Z",
  "lastCommentDate": "2020-08-21T09:40:46.000Z",
  "maintainerBlessed": false,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/46720/files",
  "hasMergeConflict": false,
  "authorIsOwner": false,
  "isFirstContribution": false,
  "popularityLevel": "Popular",
  "newPackages": [],
  "packages": [
    "bluebird"
  ],
  "files": [
    {
      "path": "types/bluebird/bluebird-tests.ts",
      "kind": "test",
      "package": "bluebird"
    },
    {
      "path": "types/bluebird/index.d.ts",
      "kind": "definition",
      "package": "bluebird"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "fail",
  "ciUrl": "https://github.com/DefinitelyTyped/DefinitelyTyped/commit/bc574372c5fd103179e12c3e1ccfcdcff5e37fb1/checks?check_suite_id=1075551965",
  "reviewersWithStaleReviews": [
    {
      "reviewedAbbrOid": "e3f6ba0",
      "reviewer": "lhecker",
      "date": "2020-08-19T10:35:25Z"
    },
    {
      "reviewedAbbrOid": "f7d9f65",
      "reviewer": "lhecker",
      "date": "2020-08-17T11:58:16Z"
    },
    {
      "reviewedAbbrOid": "f4de064",
      "reviewer": "lhecker",
      "date": "2020-08-13T17:04:32Z"
    }
  ],
  "approvalFlags": 0,
  "isChangesRequested": false
}

typescript-bot · 2020-08-13T15:52:04Z

🔔 @lhecker — 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.

danger-public · 2020-08-13T15:54:52Z

Inspecting the JavaScript source for this package found some properties that are not in the .d.ts files.
The check for missing properties isn't always right, so take this list as advice, not a requirement.

bluebird (`unpkg`)

was missing the following properties:

TypeError
RangeError
RejectionError
fulfilled
rejected

as well as these 1 other properties...

pending

Generated by 🚫 dangerJS against db02b80

typescript-bot · 2020-08-13T16:38:28Z

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

Comparison details 📊

	master	#46720	diff
Batch compilation
Memory usage (MiB)	48.2	54.6	+13.2%
Type count	7524	8832	+17%
Assignability cache size	805	868	+8%

Language service
Samples taken	2489	2489	0%
Identifiers in tests	2489	2489	0%
`getCompletionsAtPosition`
Mean duration (ms)	105.6	106.0	+0.4%
Mean CV	13.1%	12.6%
Worst duration (ms)	228.1	220.0	-3.5%
Worst identifier	fooArr	fooArr
`getQuickInfoAtPosition`
Mean duration (ms)	100.2	101.0	+0.8%
Mean CV	12.5%	12.2%	-2.4%
Worst duration (ms)	222.9	228.4	+2.5%
Worst identifier	resolve	resolve

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

lhecker

Can you explain what this PR attempts to solve? Did you perhaps accidentally specify your tuple incorrectly?
Because the example you gave works just fine for me:

const x: [string, number] = ["hello", 10]; // correct tuple declaration (important!)
Bluebird.resolve(x).all().then(([x1, x2]) => {
    let y1: string = x1;
    let y2: number = x2;
});

("Requesting changes" so the bot knows what's up.)

typescript-bot · 2020-08-13T17:05:07Z

@patricklx 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. If you disagree with the reviewer's comments, you can "dismiss" the review using GitHub's review UI. Thank you!

lhecker · 2020-08-13T17:06:45Z

Ah nevermind I figured it out... You need two .then() calls. 😄

lhecker

Finally I'd like to ask you to add a unit test about here.
I used the following code for testing (feel free to use it as well):

Bluebird.resolve(["", 0] as [string, number])
    .then(([x1, x2]) => [x1, x2])
    .then(([x1, x2]) => {
        const y1: string = x1;
        const y2: number = x2;
    });

lhecker · 2020-08-13T17:07:56Z

   * The new promise will be rejected or resolved depending on the passed `fulfilledHandler`, `rejectedHandler` and the state of this promise.
   */
  // Based on PromiseLike.then, but returns a Bluebird instance.
+  // to support static .all


This comment is not very descriptive / not very helpful. You can remove it.

lhecker · 2020-08-13T17:08:46Z

+  then<T1, T2>(onFulfill?: (value: R) => Resolvable<[T1, T2]>, onReject?: (error: any) => Resolvable<[T1, T2]>): Bluebird<[T1, T2]>;
+  then<T1, T2, T3>(onFulfill?: (value: R) => Resolvable<[T1, T2, T3]>, onReject?: (error: any) => Resolvable<[T1, T2, T3]>): Bluebird<[T1, T2, T3]>;
+  then<T1, T2, T3, T4>(onFulfill?: (value: R) => Resolvable<[T1, T2, T3, T4]>, onReject?: (error: any) => Resolvable<[T1, T2, T3, T4]>): Bluebird<[T1, T2, T3, T4]>;
+  then<T1, T2, T3, T4, T5>(onFulfill?: (value: R) => Resolvable<[T1, T2, T3, T4, T5]>, onReject?: (error: any) => Resolvable<[T1, T2, T3, T4, T5]>): Bluebird<[T1, T2, T3, T4, T5]>;


Could you please order these overloads the same way as .all is overloaded? That is: Descending from the T1-T5 variant down to the T1-T2 one.

Additionally, to make it consistent, I'd like to ask you to please rename <U> to <T1> below.

The empty newline L66 can also be removed.

typescript-bot · 2020-08-14T09:06:45Z

@lhecker Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

patricklx · 2020-08-17T07:37:36Z

changed it so it correctly resolves promises

typescript-bot · 2020-08-17T07:41:56Z

@patricklx The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

lhecker · 2020-08-17T07:57:18Z

Oh I forgot about your PR… The changes look good to me now, but unfortunately you‘re exceeding the line length limit.
You can change it to look similar to the catch method. (Don’t forget the trailing comma though!)

typescript-bot · 2020-08-17T11:48:10Z

@patricklx The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

lhecker · 2020-08-17T11:58:16Z

+      Resolvable<T1>,
+      Resolvable<T2>,
+      Resolvable<T3>
+  ], onReject?: (error: any) => [Resolvable<T1>, Resolvable<T2>, Resolvable<T3>]): Bluebird<[T1, T2, T3]>;


Nit: You forgot to add line breaks here.
Since it's only a very minor stylistic issue I'll approve your PR regardless. Of course it'd be nice if you fixed it though. 😄

typescript-bot · 2020-08-17T11:58:48Z

@patricklx The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

lhecker · 2020-08-18T14:57:56Z

The changes look just perfect. 👌

But since yesterday I've spent some time thinking about the underlying reason why you're actually submitting this change...
Isn't the problem simply that you're declaring a tuple incorrectly?

Because if I change my sample code slightly to declare tuples consistently the typings work without any changes:

function makePair<T1, T2>(x: T1, y: T2): [T1, T2] {
    return [x, y];
}

Bluebird.resolve(makePair("", 0))
    .then(([x1, x2]) => makePair(x1, x2))
    .then(([x1, x2]) => {
        let y1: string = x1;
        let y2: number = x2;
    });

The above code will compile just fine with the current Bluebird typings, since we're always declaring tuples properly.
Wouldn't that fix your issue as well without adding those overloads?

patricklx · 2020-08-18T17:55:29Z

Right, the test was not correct. But i now wonder if this changes are correct. Since it actually should not have passed. Since the array would contain the Promise in the last then. I guess i need to add something more to the static .all. something with resolvable.
I will have another look at this tomorrow.

lhecker · 2020-08-19T10:29:15Z

FYI The test currently completes successfully even without any changes to the typings.

patricklx · 2020-08-19T10:35:22Z

true, because of as [string, Promise<number>]. But is it really necessary to define the types everytime. It should be able to infer it from the types of each array item

lhecker · 2020-08-19T10:35:24Z

 });

+Bluebird.resolve()
+    .then(() => ["", Bluebird.resolve(0)])


You're not declaring a tuple here (which was a point I made earlier).

e.g. `Promise.resolve([x1, x2]).all().then([x1, x2])`

lhecker · 2020-08-19T10:40:16Z

Yeah I think I get your point, but I'm honestly not entirely sure if this PR presents a good idea anymore...
Because this inference problem you mention is present in every other Bluebird method as well. Should we now start overloading every method? I believe that would be overkill.
Furthermore the native Promise and PromiseLike types are something of a prototype for Bluebird and they don't define any such overloads either. 🤔

lhecker · 2020-08-19T10:47:48Z

To make a specific example for my previous comment: After we merged this PR, the static resolve method will continue to not infer tuple types, which is inconsistent in respect to the new then method definition. Should resolve also have such overloads? And reduce? And all the others? Now that I had a few days to think about this PR I'm not sure if it presents a direction we should pursue with these typings. What's your opinion?

patricklx · 2020-08-20T11:58:37Z

like this then?
quite new though: microsoft/TypeScript#39094

typescript-bot · 2020-08-20T12:01:47Z

@patricklx The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

typescript-bot · 2020-08-20T12:09:47Z

@patricklx The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

typescript-bot · 2020-08-20T12:22:58Z

@patricklx The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

typescript-bot · 2020-08-20T12:37:34Z

@patricklx The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

typescript-bot · 2020-08-20T12:43:37Z

@patricklx The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

lhecker · 2020-08-21T09:40:46Z

Using variadic tuple types we could simplify a lot of the redundant declarations.
I really don't mean to be rude, but as a maintainer of these typings I objectively believe, that we shouldn't deviate too far from the Promise and PromiseLike definitions within TS' stdlib.
In order for your PR to be consistent you'd have to add variadic overloads to pretty much every single method.
At the time of writing only the three all methods (including Bluebird's addition of allSettled) have overloads for tuple types, which reflects the stdlib definition of Promise.all which is similarly the only method with overloads to infer tuples.
For consistency and a reduction of "surprise" during coding I'd prefer keeping it that way and sticking with the stdlib.

typescript-bot · 2020-09-14T04:13:47Z

@patricklx I haven't seen any activity on this PR in more than 3 weeks, and this PR currently has problems that prevent it from being merged. The PR will be closed in a week if the issues aren't addressed.

typescript-bot · 2020-09-22T04:15:41Z

@patricklx To keep things tidy, we have to close PRs that aren't mergeable and don't have activity in the last month. No worries, though — please open a new PR if you'd like to continue with this change. Thank you!

patricklx changed the title ~~[Bluebird] support then -> static .all~~ [Bluebird] support then -> static .all -> then Aug 13, 2020

typescript-bot added Critical package Untested Change This PR does not touch tests labels Aug 13, 2020

typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Aug 13, 2020

lhecker suggested changes Aug 13, 2020

View reviewed changes

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Aug 13, 2020

lhecker suggested changes Aug 13, 2020

View reviewed changes

patricklx force-pushed the patch-1 branch from f4de064 to 422b3ac Compare August 14, 2020 08:43

typescript-bot removed Revision needed This PR needs code changes before it can be merged. Untested Change This PR does not touch tests labels Aug 14, 2020

typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). and removed Critical package labels Aug 15, 2020

patricklx force-pushed the patch-1 branch from 422b3ac to 1bdfcd2 Compare August 17, 2020 07:37

typescript-bot added the The CI failed When GH Actions fails label Aug 17, 2020

patricklx force-pushed the patch-1 branch from 1bdfcd2 to da101ca Compare August 17, 2020 11:43

patricklx force-pushed the patch-1 branch from da101ca to f7d9f65 Compare August 17, 2020 11:54

lhecker reviewed Aug 17, 2020

View reviewed changes

patricklx force-pushed the patch-1 branch from f7d9f65 to 0911d6b Compare August 17, 2020 12:00

patricklx force-pushed the patch-1 branch from f648221 to 56cbba8 Compare August 19, 2020 10:02

patricklx force-pushed the patch-1 branch from 56cbba8 to e3f6ba0 Compare August 19, 2020 10:33

lhecker reviewed Aug 19, 2020

View reviewed changes

fix then -> static all

db02b80

e.g. `Promise.resolve([x1, x2]).all().then([x1, x2])`

patricklx force-pushed the patch-1 branch from e3f6ba0 to db02b80 Compare August 19, 2020 10:36

typescript-bot added the The CI failed When GH Actions fails label Aug 20, 2020

use variadic

4df8db3

patricklx force-pushed the patch-1 branch from 73ded16 to 4df8db3 Compare August 20, 2020 12:04

fix

e585579

fix

4c253ef

typescript-bot added The CI failed When GH Actions fails and removed The CI failed When GH Actions fails labels Aug 20, 2020

fix

bc57437

typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Sep 22, 2020

typescript-bot closed this Sep 22, 2020

Conversation

patricklx commented Aug 13, 2020

Uh oh!

typescript-bot commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1 package in this PR

Code Reviews

Status

Inactive

Uh oh!

typescript-bot commented Aug 13, 2020

Uh oh!

danger-public commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

bluebird (unpkg)

Uh oh!

typescript-bot commented Aug 13, 2020

Uh oh!

lhecker left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Aug 13, 2020

Uh oh!

lhecker commented Aug 13, 2020

Uh oh!

lhecker left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Aug 14, 2020

Uh oh!

patricklx commented Aug 17, 2020

Uh oh!

typescript-bot commented Aug 17, 2020

Uh oh!

lhecker commented Aug 17, 2020

Uh oh!

typescript-bot commented Aug 17, 2020

Uh oh!

lhecker Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Aug 17, 2020

Uh oh!

lhecker commented Aug 18, 2020

Uh oh!

patricklx commented Aug 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lhecker commented Aug 19, 2020

Uh oh!

patricklx commented Aug 19, 2020

Uh oh!

lhecker Aug 19, 2020

Choose a reason for hiding this comment

Uh oh!

lhecker commented Aug 19, 2020

Uh oh!

lhecker commented Aug 19, 2020

Uh oh!

patricklx commented Aug 20, 2020

Uh oh!

typescript-bot commented Aug 20, 2020

Uh oh!

typescript-bot commented Aug 20, 2020

Uh oh!

typescript-bot commented Aug 20, 2020

Uh oh!

typescript-bot commented Aug 20, 2020

Uh oh!

typescript-bot commented Aug 20, 2020

Uh oh!

typescript-bot commented Aug 13, 2020 •

edited

Loading

danger-public commented Aug 13, 2020 •

edited

Loading

bluebird (`unpkg`)

lhecker left a comment •

edited

Loading

lhecker left a comment •

edited

Loading

lhecker Aug 13, 2020 •

edited

Loading

patricklx commented Aug 18, 2020 •

edited

Loading