Make methods return bluebird promise instead native promise by BnayaZil · Pull Request #76 · benjamingr/bluebird-api

BnayaZil · 2017-06-14T20:52:36Z

Solve #72
#goodnessSquad

benjamingr

Can you please write tests that assert that the promise is bluebird-api and assert the changes you make change those tests from failing to passing?

benjamingr · 2017-06-14T23:10:27Z

        })());
    };

-    Bluebird.call = (o, ...args) => Bluebird.resolve(o).call(...args);


This change seems unnecessary.

benjamingr · 2017-06-14T23:10:40Z

@@ -1,5 +1,8 @@
 module.exports = (Bluebird) => {
-    Bluebird.any = (prom, n) => Bluebird.resolve(prom).any();


This change seems unnecessary.

it wasn't..

But Bluebird.resolve already returns a bluebird instance, so why would wrapping help?

Because you resolve before you do the any, you need to resolve the any values.

But .any() already returns a bluebird promise.

benjamingr · 2017-06-14T23:10:50Z

    Bluebird.prototype.delay = function delay(ms) {
        return this.then(obj => new Bluebird((onFulfilled) => setTimeout(() => onFulfilled(obj), ms)));
    }
-    Bluebird.delay = (ms, o) => Bluebird.resolve(o).delay(ms);


This change seems unnecessary.

benjamingr · 2017-06-14T23:11:29Z

-      })());
-    };
+	Bluebird.prototype.each = function each(iterator) {
+		return Bluebird.resolve((async () => { 


Was anything actually changed here?

benjamingr · 2017-06-14T23:11:39Z

@@ -1,6 +1,9 @@
 const util = require("./util");
 module.exports = (Bluebird) => {
-    Bluebird.filter = (x, predicate, opts) => Bluebird.resolve(x).filter(predicate, opts);


This change seems unnecessary.

it wasn't..

But Bluebird.resolve already returns a bluebird instance, so why would wrapping help?

Because you resolve before you do the filter, you need to resolve the filter values.

But p.filter(predicate) already returns a bluebird promise.

They are not, filter, each and map didn't return bluebird promise, I have tested it.
I can move bluebird resolve wrapper to the prototype functions if it seems cleaner to you.

Can you add a test so we can see?

benjamingr · 2017-06-14T23:12:03Z

      })());
    };

-    Bluebird.mapSeries = (promise, iterator) => Bluebird.resolve(promise).mapSeries(iterator);


This change seems unnecessary.

benjamingr · 2017-06-14T23:12:08Z

            return ret;
        })());
    };
-    Bluebird.props = o => Bluebird.resolve(o).props();


This change seems unnecessary.

benjamingr · 2017-06-14T23:12:16Z

      })());
    };

-    Bluebird.reduce = (promise, reducer, initialValue) => Bluebird.resolve(promise).reduce(reducer, initialValue);


This change seems unnecessary.

benjamingr · 2017-06-14T23:12:22Z

@@ -1,5 +1,8 @@
 module.exports = (Bluebird) => {
-    Bluebird.some = (prom, n) => Bluebird.resolve(prom).some(n);


This change seems unnecessary.

benjamingr · 2017-06-14T23:12:28Z

            return winner;
        })());
    }
-    Bluebird.timeout = (ms, rejectDesc, o) => Bluebird.resolve(o).delay(ms)


This change seems unnecessary.

BnayaZil · 2017-06-15T20:34:59Z

The setScheduler tests failed before my changes, except them, all tests are passing, I added tests for checking if the returned promise is an instance of Bluebird, I hope using instanceof it's fine, I checked it in positive and negative and it seems ok.

BnayaZil · 2017-07-01T20:55:09Z

@benjamingr I add tests to make sure the promises from the prototype returns a non-bluebird instance.
As I suggested before, If the pattern is to wrap the methods with bluebird object in the prototype I can move the wrapper and get rid of the redundant test.

Make methods return bluebird promise instead native promise.

84b8702

benjamingr reviewed Jun 14, 2017

View reviewed changes

BnayaZil added 2 commits June 15, 2017 23:26

Make methods return bluebird promise instead native promise.

a327e54

resolve conflicts

1320acc

Add tests to validate non-bluebird instance

6cc65ca

		@@ -1,5 +1,8 @@
		module.exports = (Bluebird) => {
		Bluebird.any = (prom, n) => Bluebird.resolve(prom).any();

		@@ -1,5 +1,8 @@
		module.exports = (Bluebird) => {
		Bluebird.some = (prom, n) => Bluebird.resolve(prom).some(n);

Conversation

BnayaZil commented Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BnayaZil commented Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BnayaZil commented Jul 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

BnayaZil commented Jun 14, 2017 •

edited

Loading

BnayaZil commented Jun 15, 2017 •

edited

Loading