Bluebird: New warnings in 3.0

[vitaly-t]

All my jasmine tests started producing over thousand warnings, after upgrading from 2.x to 3.x

• Warning: a promise was rejected with a non-error: [object Array]
• Warning: a promise was created in a handler but none were returned from it

And this is all for a library that passes tests without warnings against every promise library there is out there, including Bluebird 2.x

I went through the code referred to by the warnings, and none of them make sense to me, i.e. they look completely wrong.

[petkaantonov]

They are documented at http://bluebirdjs.com/docs/warning-explanations.html

[vitaly-t]

Wow, those two are shocking changes...

• Warning: a promise was rejected with a non-error: [object Array]

That's trying to create your own standard against what JavaScript supports, like throw;, which is a perfectly valid command, and is not meant to produce any warning, and the same is expected from any promise library.

• Warning: a promise was created in a handler but none were returned from it

In my code and in other people's it is all over the place when undefined is used as a perfectly good value to either return or resolve with. Your warning here is breaking a perfectly good code pattern, and nobody is gonna like that.

In all, I think you will be crucified for these two warnings, unless you turn around quickly and make them optional, that which I strongly suggest.

[petkaantonov]

They _are_ optional.

[vitaly-t]

Optional - means OFF by default.

Also, I just noticed that even switching over to production mode doesn't get rid of them.

[petkaantonov]

They are off by default as well.

[vitaly-t]

Not according to my all tests, and I do not even use that Promise.config at all.

[petkaantonov]

Read the documentation fully, you can also set environment variables that enable them

[vitaly-t]

I have, and now I am beginning to suspect where the problem lies...

Back a few month you did a fix in 2.x to make sure that when we have BLUEBIRD_DEBUG=0, that's disabling debugging.

It seems to me that change failed to make it into 3.x, hence the problem.

[benjamingr]

This sounds like bluebird found a ton of errors in your code - I suggest that you handle them by fixing them :)

[vitaly-t]

@petkaantonov I can now confirm that's exactly the problem. I had BLUEBIRD_DEBUG=0, and I had to delete that variable completely to make the warnings disappear. This breaks the 2.x pattern where setting BLUEBIRD_DEBUG=0 was disabling the debug mode, which is a very important feature.

@benjamingr please don't even start! :)

[benjamingr]

@vitaly-t bluebird is an opinionated promise library, our philosophy is to make things that work for 99% of people 100% of the time. If you don't like the _opinion_ of the library that's fine - you're more than welcome to fork it it's an open source project.

Rejecting with non-errors is an obvious bug, an array is not an error - it doesn't have a stack trace and it'll make your life hell to throw arrays (or reject with them) in large production code bases.

Creating promises in then but not handling them is an obvious bug as well - most likely a deferred anti-pattern which leaves room for a lot of errors.

[petkaantonov]

Based on source code of the 2.x branch, that would actually enable debugging as well. 2.x doesnt have warnings thats why you didnt see them

[vitaly-t]

@petkaantonov Not only I debugged that code to see the var debugging is set correctly in debuggability.js, based on all combinations of variables, but also my code relied a lot on BLUEBIRD_DEBUG=0 to disable long-stack tracing as well, because in my special test cases they were getting in the way, while running in development mode.

So no, that would not enable debugging, well tested back with 2.x.

@benjamingr obviously, we have a different notion about the obvious, and let's leave at that :) I appreciate you're pepped up for a morning metaphysics seminar, but it's night for me here, and I won't bite :)

[vitaly-t]

@petkaantonov As per that document, the new warnings pop up even when the environment is simply set to development. That's not exactly OFF by default. Which means every developer is going to be hitting those warnings now, and wonder what's going on...

[benjamingr]

Well, only ones with errors in their code. People who make reasonable usage of promises should be fine - or they could read the documentation and disable warnings if they like taking walks on the wild side.

[vitaly-t]

I can see it all got fixed now. Thanks a bunch! :+1:

[marneborn]

I needed to change all of my async jasmine tests to look like this to get rid of the warnings caused by jasmine's async chaining returning an undef.

describe("suite", function () {
    var BPromise = require('bluebird');

    function myAsync () {
        return new BPromise(function (resolve, reject) {
            resolve();
        });
    }

    it("should pass", function (done) {
        myAsync()
        .then(function () {
            expect(true).toBe(true, 1);
            return null;
        })
        .finally(done);
    });

    it("should pass", function (done) {
        myAsync()
        .then(function () {
            expect(true).toBe(true, 2);
            return null;
        })
        .finally(done);
    });

});

The return of null in the body of the test is important even though that block is synchronous.
The undef is passed through finally which creates a promise inside of the done function.

Same for my async beforeEach's and afterEach's.

[vitaly-t]

@marneborn as per the posts above, set environment variable BLUEBIRD_DEBUG=0, and it's all sorted.

[petkaantonov]

@vitaly-t it's better to do BLUEBIRD_WARNINGS=0 only so you don't lose long stack traces

[vitaly-t]

@petkaantonov thanks! didn't know that was supported.

[marneborn]

I didn't do a good job of explaining myself, sorry.
I like the warnings and don't want to turn them off, this would have flagged a couple of bugs early for me.

The standard advice for testing promises in jasmine is to do something like this

it("should test something", function (done) {
    return createPromise()
    .then(function (res) {
        /* check results */
    })
    .then(done, done.fail);
})

Where done is a function that runs the next step in the queue.
If something in the queue creates a promise, bluebird will flag warnings because the done function returns undefined. This function is defined within jasmine.

For example, in this -spec.js there is no Promise created in my code that isn't returned, but warnings will be flagged anyway. At least I don't see anything that should be flagged.

describe("suite", function () {
    var BPromise = require('bluebird');

    function myAsync () {
        return new BPromise(function (resolve, reject) {
            resolve();
        })
        .then(function () {
            return new BPromise(function (resolve, reject) {
                resolve();
            });
        });
    }

    it("should test something", function (done) {
        return myAsync()
        .then(function () {
            expect(true).toBe(true, 1);
        })
        .then(done, done.fail);
    });
    it("should test something", function (done) {
        return myAsync()
        .then(function () {
            expect(true).toBe(true, 1);
        })
        .then(done, done.fail);
    });
});

If I want to check my code with these warnings, I need to make sure that all of the handlers return null, not undefined. So, either jasmine needs put a return in the done function or I need put wrappers around done and done.fail if calling them in .then or .catch:

it("should check something", function (done) {
    return createPromise()
    .then(function (res) {
        /* check results */
    })
    .then(function () {
        done();
        return null;
    }, function () {
        done.fail();
        return null;
    })
})

Or I need to make sure that the .then and .catch before a finally returns null, even if they are synchronous.

it("should test something", function (done) {
    return createPromise()
    .then(function (res) {
        /* check results */
        return null; // Needed because .finally passes this value through
    })
    .catch(function (err) {
        /* check error */
        return null; // Needed because .finally passes this value through
    })
    .finally(done);
})

[petkaantonov]

I have experimented with a design that allows knowing if the handler was immediately creating a promise but not returning it. However, this has tremendous performance cost and it is trading off false positives for false negative. Additionally, false positives can always be dealth with by returning null while false negatives cannot be dealt with at all.

Example of false negative:

function returnsPromise() {
    return Promise.resolve();
}


somePromise.then(function() {
    // Forgot to return but no warning
    // because the promise is not immediately created here.
    returnsPromise();
});

[CorpusCallosum]

Hi I'm getting this same warnings in my code, not sure why as I did not write it originally, but here it is:

ardDataManager.getCardDataPromise = function(){
    console.log('cardDataManager.getCardData...');

    // read th emain bundle path and return the promise
    return new Promise(function(resolve, reject){

        RNFS.readDir(RNFS.MainBundlePath).then((result)=>{
                // console.log('cardDataManager.getCardDataPromise:then...', result);

                // let's find the one named cardData
                var cardData = result.find((el,index,orig)=>{
                    return el.name==='cardData'
                });

                // read the card content list json and return the promise
                RNFS.readFile(cardData.path+'/data/card-content-list.json')
                    .then(JSON.parse)
                    .then((cardList)=>{
                        // console.log('cardData.readDir...',cardList);
                        resolve({
                            docRoot:cardData.path,
                            cardList:cardList
                        });
                    });
            });

    });

}

How can I get rid of these waning or suppress them?
Where do I put the option BLUEBIRD_WARNINGS=0 ?
I cannot find it in the documentation.

Thanks.

[thebarefootdev]

I have hundreds of lines of such warnings despite implementing the promises as requested, and I haven't been able to get rid of them. Further more, within our deployment strategy we do not set environment variables, as or CI procedures aren't good at using them, as well as having a wide range of deployment platforms.

This is hugely frustrating, and I dont think has been fully addressed in the documentation. Please advise

[benjamingr]

I have hundreds of lines of such warnings despite implementing the promises as requested, and I haven't been able to get rid of them.

Can you show us a single example you believe is unjustified?