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

Warning when native promise implementation is replaced with bluebird #588

Open
k0pernikus opened this issue Feb 6, 2017 · 2 comments
Open
Labels

Comments

@k0pernikus
Copy link

In a node application using highland@2.10.1 I have replaced the native promise implemenation with bluebird@3.4 due to performance consideration via:

global.Promise = Bluebird;

While highland seems to be working just as fine as before, I lot of warning with the content:

(node:1428) Warning: a promise was created in a handler at usr/src/marketing-tasks/node_modules/highland/lib/index.js:517:24 but was not returned from it, see http://goo.gl/rRqMUw

The offending code is index.js in line 517:

function promiseStream(promise) {
    if (_.isFunction(promise['finally'])) { // eslint-disable-line dot-notation
        // Using finally handles also bluebird promise cancellation
        return _(function (push) {
            promise.then(function (value) {
                return push(null, value); // it complains here
            },
                function (err) {
                    return push(err);
                })['finally'](function () { // eslint-disable-line dot-notation
                    return push(null, nil);
                });
        });
    }

I am unsure if this is either an issue with how highland.js creates promises or if bluebird's warning system fires a false positive.

This is me asking: Does bluebird's warning has any merit for highland.js? Or should this be an issue on bluebird's side?

@k0pernikus k0pernikus changed the title Warning when promise library is replaced with bluebird Warning when native promise implementation is replaced with bluebird Feb 6, 2017
vqvu added a commit to vqvu/highland that referenced this issue Feb 7, 2017
We signify uncaught errors by doing this.emit('error'). This throws an
error if there are no bound handlers. Previously, for promise-based
streams, such thrown errors may propagate up to the promise and be
swallowed by whatever promise implementation the user is using.

We can push data asynchonously to fix this problem. While we're at it,
also return null from promise handlers so that bluebird doesn't
complain. See caolan#588.
@vqvu vqvu added the bug label Feb 7, 2017
@vqvu
Copy link
Collaborator

vqvu commented Feb 7, 2017

It's a false positive, but we should be able to suppress the warning on our side by just returning null.

vqvu added a commit to vqvu/highland that referenced this issue Feb 7, 2017
@vqvu
Copy link
Collaborator

vqvu commented Feb 7, 2017

Try out v2.10.2 and let me know if you're still seeing these warnings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants