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

Warning: a promise was rejected with a non-error: [object Error] #990

Closed
ColCh opened this Issue Feb 3, 2016 · 29 comments

Comments

Projects
None yet
@ColCh

ColCh commented Feb 3, 2016

A warning is fired when I reject a Promise with an new Error('...'), but Promise and Error are from different Window 's (e.g. window.opener and window child - from window.open or iframe.contentWindow).

Of cource, it means, that rejectReason instanceof Error returns false, because Error is instance of iframe.contentWindow.Error (not of window.Error)

@ColCh

This comment has been minimized.

Show comment
Hide comment
@ColCh

ColCh Feb 3, 2016

Upd: To hot-fixing this issue.

You can create another instance for Error and proxy all props from previous to new, and reject a promise with created Error.

            if (err) {
                const proxiedError = new Error();
                proxiedError.message = err.message;
                proxiedError.stack = err.stack;
                reject(proxiedError);
            } else {
                resolve(res);
            }

However, bluebird should use [[Class]] prop to determine errors (Object.prototype.toString), isn't it? Or I'm wrong?

ColCh commented Feb 3, 2016

Upd: To hot-fixing this issue.

You can create another instance for Error and proxy all props from previous to new, and reject a promise with created Error.

            if (err) {
                const proxiedError = new Error();
                proxiedError.message = err.message;
                proxiedError.stack = err.stack;
                reject(proxiedError);
            } else {
                resolve(res);
            }

However, bluebird should use [[Class]] prop to determine errors (Object.prototype.toString), isn't it? Or I'm wrong?

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Feb 3, 2016

Collaborator

What about error subclasses?
Honestly I'm fine with checking if it's possible to attach a stack to it.

Collaborator

benjamingr commented Feb 3, 2016

What about error subclasses?
Honestly I'm fine with checking if it's possible to attach a stack to it.

@ColCh

This comment has been minimized.

Show comment
Hide comment
@ColCh

ColCh Feb 3, 2016

For subclasses - same. Checked with EvalError and ReferenceError.

@benjamingr do you need a PR with failing test case?

ColCh commented Feb 3, 2016

For subclasses - same. Checked with EvalError and ReferenceError.

@benjamingr do you need a PR with failing test case?

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Feb 3, 2016

Collaborator

Let's be more concrete. Can you show me a piece of code that takes something and determines if it's an error?

Collaborator

benjamingr commented Feb 3, 2016

Let's be more concrete. Can you show me a piece of code that takes something and determines if it's an error?

@ColCh

This comment has been minimized.

Show comment
Hide comment
@ColCh

ColCh Feb 3, 2016

Sure. Consider following snippet:

export async function evalInWindowAsync (win, func/*nodeClb, jQuery*/, additionalArgs = []) {
    return new Promise((resolve, reject) => {

        let args = [];
        const argsId = _.uniqueId('evalInWindow_arg_holder_');

        const callback = function(err, res) {

            delete win[argsId];

            if (err) {
                const proxiedError = new EvalError();
                proxiedError.message = err.message;
                proxiedError.stack = err.stack;
                reject(proxiedError);
            } else {
                resolve(res);
            }
        };

        args.push(callback);
        args = args.concat(additionalArgs);
        win[argsId] = args;

        const evalArgs = [`window["${argsId}"][0]`, `window.jQuery`];

        for (var i = 1; i < args.length; i++) {
            evalArgs.push(`window["${argsId}"]["${i}"]`);
        }

        const functionString = _.isString(func) ? functionString : func.toString();
        const safeFunctionString = `function (done) {'use strict'; try { (${functionString}).apply(this, arguments); } catch (e) { done(e); } }`;
        const evalString = `;(${safeFunctionString})(${evalArgs.join(',')});`;
        win.eval(evalString);
    });
}

Code, which determines - resolve or reject a Promise. It's a Node-style callback.

        const callback = function(err, res) {

            // ...

            if (err) {
                const proxiedError = new EvalError();
                proxiedError.message = err.message;
                proxiedError.stack = err.stack;
                reject(proxiedError);
            } else {
                resolve(res);
            }
        };

And demo itself (snippet from a test case with chai-as-promised):

            const win = frame.contentWindow;

            const stub = function (done, $) { throw new Error('an error'); }

            return evalInWindowAsync(win, stub).should
                .eventually.be.rejectedWith('an error');

stub here becomes a string, get's evalled -> Error becomes frame.contentWindow.Error.
Test case passes, but bluebird fires warning in console.

ColCh commented Feb 3, 2016

Sure. Consider following snippet:

export async function evalInWindowAsync (win, func/*nodeClb, jQuery*/, additionalArgs = []) {
    return new Promise((resolve, reject) => {

        let args = [];
        const argsId = _.uniqueId('evalInWindow_arg_holder_');

        const callback = function(err, res) {

            delete win[argsId];

            if (err) {
                const proxiedError = new EvalError();
                proxiedError.message = err.message;
                proxiedError.stack = err.stack;
                reject(proxiedError);
            } else {
                resolve(res);
            }
        };

        args.push(callback);
        args = args.concat(additionalArgs);
        win[argsId] = args;

        const evalArgs = [`window["${argsId}"][0]`, `window.jQuery`];

        for (var i = 1; i < args.length; i++) {
            evalArgs.push(`window["${argsId}"]["${i}"]`);
        }

        const functionString = _.isString(func) ? functionString : func.toString();
        const safeFunctionString = `function (done) {'use strict'; try { (${functionString}).apply(this, arguments); } catch (e) { done(e); } }`;
        const evalString = `;(${safeFunctionString})(${evalArgs.join(',')});`;
        win.eval(evalString);
    });
}

Code, which determines - resolve or reject a Promise. It's a Node-style callback.

        const callback = function(err, res) {

            // ...

            if (err) {
                const proxiedError = new EvalError();
                proxiedError.message = err.message;
                proxiedError.stack = err.stack;
                reject(proxiedError);
            } else {
                resolve(res);
            }
        };

And demo itself (snippet from a test case with chai-as-promised):

            const win = frame.contentWindow;

            const stub = function (done, $) { throw new Error('an error'); }

            return evalInWindowAsync(win, stub).should
                .eventually.be.rejectedWith('an error');

stub here becomes a string, get's evalled -> Error becomes frame.contentWindow.Error.
Test case passes, but bluebird fires warning in console.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Feb 3, 2016

Owner

[[Class]] Doesn't work with subclasses:

function MyError() {
    Error.apply(this, arguments);
    Error.captureStackTrace(this, MyError);
}
MyError.prototype = Object.create(Error.prototype);
MyError.prototype.constructor = Error;

var error = new MyError();
({}.toString.call(error)) //[Object object]
Owner

petkaantonov commented Feb 3, 2016

[[Class]] Doesn't work with subclasses:

function MyError() {
    Error.apply(this, arguments);
    Error.captureStackTrace(this, MyError);
}
MyError.prototype = Object.create(Error.prototype);
MyError.prototype.constructor = Error;

var error = new MyError();
({}.toString.call(error)) //[Object object]
@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Feb 3, 2016

Owner

@benjamingr I guess we could check for presence of .name, .message and .stack

Owner

petkaantonov commented Feb 3, 2016

@benjamingr I guess we could check for presence of .name, .message and .stack

@ColCh

This comment has been minimized.

Show comment
Hide comment
@ColCh

ColCh Feb 3, 2016

@petkaantonov almost forgot about that. Sorry for misinfo

ColCh commented Feb 3, 2016

@petkaantonov almost forgot about that. Sorry for misinfo

@mjomble

This comment has been minimized.

Show comment
Hide comment
@mjomble

mjomble Feb 10, 2016

Contributor

I'm also getting the "promise was rejected with a non-error" warning for regular new Error(...) type errors when the code runs inside PhantomJS with Bluebird 3.x.

It worked fine with Bluebird 2.9.x

Contributor

mjomble commented Feb 10, 2016

I'm also getting the "promise was rejected with a non-error" warning for regular new Error(...) type errors when the code runs inside PhantomJS with Bluebird 3.x.

It worked fine with Bluebird 2.9.x

@sukima

This comment has been minimized.

Show comment
Hide comment
@sukima

sukima Mar 21, 2016

Contributor

For anyone who finds this though hair pulling and Google. This bug is also relevant for cases in Node.js that use vm.runInThisContext. Spent two days trying to diagnose why my custom Error object's prototype was undefined! After deleting several console.logs I put in Bluebird source and then upgrading to 3.3.4 it all got fixed thanks to commit e77224c.

Contributor

sukima commented Mar 21, 2016

For anyone who finds this though hair pulling and Google. This bug is also relevant for cases in Node.js that use vm.runInThisContext. Spent two days trying to diagnose why my custom Error object's prototype was undefined! After deleting several console.logs I put in Bluebird source and then upgrading to 3.3.4 it all got fixed thanks to commit e77224c.

@liorcode

This comment has been minimized.

Show comment
Hide comment
@liorcode

liorcode May 1, 2016

Checking if message and name are string (e77224c) causes a new bug since 'message' is actually optional. so now rejecting with simply "new MyError()" (my error extends Error..) doesn't work anymore (causes the "rejected with non error [object Error]" warning). I realize it is difficult to effectively check if an object is an error, so how about trying both the old way and the new:

function isErrorLike(obj) {
    return obj !== null &&
           typeof obj === "object" &&
           typeof obj.message === "string" &&
           typeof obj.name === "string";
}

function canAttachTrace(obj) {
    return (obj instanceof Error || isErrorLike(obj)) && es5.propertyIsWritable(obj, "stack");
}

what do you think?

liorcode commented May 1, 2016

Checking if message and name are string (e77224c) causes a new bug since 'message' is actually optional. so now rejecting with simply "new MyError()" (my error extends Error..) doesn't work anymore (causes the "rejected with non error [object Error]" warning). I realize it is difficult to effectively check if an object is an error, so how about trying both the old way and the new:

function isErrorLike(obj) {
    return obj !== null &&
           typeof obj === "object" &&
           typeof obj.message === "string" &&
           typeof obj.name === "string";
}

function canAttachTrace(obj) {
    return (obj instanceof Error || isErrorLike(obj)) && es5.propertyIsWritable(obj, "stack");
}

what do you think?

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr May 1, 2016

Collaborator

@petkaantonov ^ sounds good to me generally.

Collaborator

benjamingr commented May 1, 2016

@petkaantonov ^ sounds good to me generally.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov May 2, 2016

Owner

Even if you don't pass message, it's still the empty string. Maybe your transpiler doesn't do subclassing correctly?

Owner

petkaantonov commented May 2, 2016

Even if you don't pass message, it's still the empty string. Maybe your transpiler doesn't do subclassing correctly?

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR May 2, 2016

Contributor

@petkaantonov subclass is often done like this:

// Example for Node.js
const util = require('util');

function SubError (message) {
    Error.captureStackTrace(this, this.constructor);
    // Stuff
    this.message = message; 
}
util.inherits(SubError, Error);

var err = new SubError ();
console.log(typeof err.message); // => undefined

Most people probably don't set a default value in such a case.

Contributor

BridgeAR commented May 2, 2016

@petkaantonov subclass is often done like this:

// Example for Node.js
const util = require('util');

function SubError (message) {
    Error.captureStackTrace(this, this.constructor);
    // Stuff
    this.message = message; 
}
util.inherits(SubError, Error);

var err = new SubError ();
console.log(typeof err.message); // => undefined

Most people probably don't set a default value in such a case.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov May 2, 2016

Owner

That's not a subclass (it would not be instanceof Error) and captureStackTrace is proprietary.

Owner

petkaantonov commented May 2, 2016

That's not a subclass (it would not be instanceof Error) and captureStackTrace is proprietary.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr May 2, 2016

Collaborator

So what if it's proprietary? Can we detect it?

Collaborator

benjamingr commented May 2, 2016

So what if it's proprietary? Can we detect it?

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr May 2, 2016

Collaborator

Can't we just check for .stack?

Collaborator

benjamingr commented May 2, 2016

Can't we just check for .stack?

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR May 2, 2016

Contributor

I updated my example to inherit from Error. I implied a Node.js inheritance.

Contributor

BridgeAR commented May 2, 2016

I updated my example to inherit from Error. I implied a Node.js inheritance.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov May 2, 2016

Owner

You forgot to call super constructor, captureStackTrace doesnt do it and doesnt work in all browsers

Owner

petkaantonov commented May 2, 2016

You forgot to call super constructor, captureStackTrace doesnt do it and doesnt work in all browsers

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov May 2, 2016

Owner

.stack is non standard so no

Owner

petkaantonov commented May 2, 2016

.stack is non standard so no

@wayofthefuture

This comment has been minimized.

Show comment
Hide comment
@wayofthefuture

wayofthefuture Sep 12, 2016

            .fail((xhr, err) => {
                let proxy = new Error();
                proxy.message = err || 'error is null';
                proxy.name = 'ajax error';
                reject(proxy);
            });

wayofthefuture commented Sep 12, 2016

            .fail((xhr, err) => {
                let proxy = new Error();
                proxy.message = err || 'error is null';
                proxy.name = 'ajax error';
                reject(proxy);
            });
@gabegorelick

This comment has been minimized.

Show comment
Hide comment
@gabegorelick

gabegorelick Oct 2, 2017

Contributor

Apologies for commenting on an old issue, but @BridgeAR's comment still holds for ES classes. Here's an example from Sequelize:

class BaseError extends Error {
  constructor(message) {
    super(message);
    this.name = 'SequelizeBaseError';
    this.message = message;
    Error.captureStackTrace(this, this.constructor);
  }
}

Now, why they have this.message = message is a good question (perhaps a holdover from when this code didn't use ES classes?). But the fact is there's actual code out there that instantiates Errors like this that will cause Bluebird to issue spurious warnings, even though new BaseError() instanceof Error.

@petkaantonov Would you reconsider adding back the obj instanceof Error as an additional check? Something like:

function isError(obj) {
    return obj instanceof Error ||
           (obj !== null &&
           typeof obj === "object" &&
           typeof obj.message === "string" &&
           typeof obj.name === "string");
}

Happy to open a PR.

Contributor

gabegorelick commented Oct 2, 2017

Apologies for commenting on an old issue, but @BridgeAR's comment still holds for ES classes. Here's an example from Sequelize:

class BaseError extends Error {
  constructor(message) {
    super(message);
    this.name = 'SequelizeBaseError';
    this.message = message;
    Error.captureStackTrace(this, this.constructor);
  }
}

Now, why they have this.message = message is a good question (perhaps a holdover from when this code didn't use ES classes?). But the fact is there's actual code out there that instantiates Errors like this that will cause Bluebird to issue spurious warnings, even though new BaseError() instanceof Error.

@petkaantonov Would you reconsider adding back the obj instanceof Error as an additional check? Something like:

function isError(obj) {
    return obj instanceof Error ||
           (obj !== null &&
           typeof obj === "object" &&
           typeof obj.message === "string" &&
           typeof obj.name === "string");
}

Happy to open a PR.

gabegorelick added a commit to gabegorelick/sequelize that referenced this issue Oct 2, 2017

fix(errors): spurious Bluebird warnings
The way Sequelize instantiates some of its Error subclasses
confuses Bluebird into thinking that Promises are rejected
with non-Error objects.

See petkaantonov/bluebird#990

@gabegorelick gabegorelick referenced this issue Oct 2, 2017

Merged

fix(errors): spurious Bluebird warnings #8408

5 of 5 tasks complete
@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Oct 3, 2017

Owner

@gabegorelick yeah should be no harm in it

Owner

petkaantonov commented Oct 3, 2017

@gabegorelick yeah should be no harm in it

gabegorelick added a commit to gabegorelick/bluebird that referenced this issue Oct 3, 2017

janmeier added a commit to sequelize/sequelize that referenced this issue Oct 4, 2017

fix(errors): spurious Bluebird warnings (#8408)
The way Sequelize instantiates some of its Error subclasses
confuses Bluebird into thinking that Promises are rejected
with non-Error objects.

See petkaantonov/bluebird#990
@jalmansa88

This comment has been minimized.

Show comment
Hide comment
@jalmansa88

jalmansa88 Aug 8, 2018

I am experiencing this issue using aurelia-http-client. Even configure the client using a HttpInterceptor returning a new Error per errorCode case, I still see the warning un browser console.

I am not able to remove this warning.
Help is appreciate.

jalmansa88 commented Aug 8, 2018

I am experiencing this issue using aurelia-http-client. Even configure the client using a HttpInterceptor returning a new Error per errorCode case, I still see the warning un browser console.

I am not able to remove this warning.
Help is appreciate.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Aug 8, 2018

Collaborator

@jalmansa88 well - for start you can turn it off directly, it's very unlikely we'll be able to help you without a MCVE

Collaborator

benjamingr commented Aug 8, 2018

@jalmansa88 well - for start you can turn it off directly, it's very unlikely we'll be able to help you without a MCVE

@atifsyedali

This comment has been minimized.

Show comment
Hide comment
@atifsyedali

atifsyedali Aug 16, 2018

Is there a way to turn off just this specific warning but not other warnings?

atifsyedali commented Aug 16, 2018

Is there a way to turn off just this specific warning but not other warnings?

@benjamingr

This comment has been minimized.

Show comment
Hide comment
Collaborator

benjamingr commented Aug 16, 2018

@jalmansa88

This comment has been minimized.

Show comment
Hide comment
@jalmansa88

jalmansa88 commented Aug 18, 2018

Thanks @benjamingr

@atifsyedali

This comment has been minimized.

Show comment
Hide comment
@atifsyedali

atifsyedali Aug 18, 2018

The only warning suppression flag I see there is for when you forget to return a promise.

wForgottenReturn is the only warning type that can be separately configured. The corresponding environmental variable key is BLUEBIRD_W_FORGOTTEN_RETURN.

Maybe I am missing something? In any case, I only get the warning on startup when es6-shim tries to initialize and test the features supported by the Promise impl in the env. See https://github.com/paulmillr/es6-shim/blob/8583cbe2186ec12bfbf4218ccbce7fc73f96de87/es6-shim.js#L2654

I'll suppress this myself so no worries. Just wanted to know if there is a way to turn it off already.

atifsyedali commented Aug 18, 2018

The only warning suppression flag I see there is for when you forget to return a promise.

wForgottenReturn is the only warning type that can be separately configured. The corresponding environmental variable key is BLUEBIRD_W_FORGOTTEN_RETURN.

Maybe I am missing something? In any case, I only get the warning on startup when es6-shim tries to initialize and test the features supported by the Promise impl in the env. See https://github.com/paulmillr/es6-shim/blob/8583cbe2186ec12bfbf4218ccbce7fc73f96de87/es6-shim.js#L2654

I'll suppress this myself so no worries. Just wanted to know if there is a way to turn it off already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment