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

commandFactory is swallowing exceptions #2187

Closed
katemihalikova opened this issue Feb 25, 2016 · 5 comments
Closed

commandFactory is swallowing exceptions #2187

katemihalikova opened this issue Feb 25, 2016 · 5 comments

Comments

@katemihalikova
Copy link

This changed line in commandFactory prevents errors thrown during emitting 'end' event in .then handler to propagate and those errors are swallowed.

Code used:

bower.commands.list().on('end', function() {throw new Error('test');});

In v1.7.0 and earlier, error is thrown:
In v1.7.1 and later, nothing is thrown and the error is swallowed.
Expected behaviour:
Error is rethrown as before or emitted via the logger logger.emit('error', error);.

@faceleg
Copy link
Member

faceleg commented Feb 29, 2016

Thanks for reporting this.

When I run your example:

var bower = require('bower');
bower.commands.list().on('end', function() {throw new Error('test');});

With 1.7.1, the code runs then exits. With 1.7.2 it seems to hang forever. Is this similar to what you reported?

@katemihalikova
Copy link
Author

The change was done between v1.7.0 and v1.7.1, sorry for that mistake I did in the description. I have updated the issue. Please try the snippet above in those two versions to see the difference.

@zbnauj
Copy link
Member

zbnauj commented Mar 24, 2016

I just tried the code above with 1.7.7 and the code runs then exits.

@katemihalikova
Copy link
Author

For me, the problem is not hanging, but Error swallowing. When an exception occurs in the callback, bower should not ignore it.

$ npm i bower@1.7.0
...
$ node -e "require('bower').commands.list().on('end', function() {throw new Error('test');});"
C:\test\node_modules\bower\node_modules\q\q.js:155
                throw e;
                ^

Error: test
    at Logger.<anonymous> (C:\test\test.js:5:52)
    at emitOne (events.js:77:13)
    at Logger.emit (events.js:169:7)
    at Logger.emit (C:\test\node_modules\bower\node_modules\bower-logger\lib\Logger.js:29:39)
    at C:\test\node_modules\bower\lib\commands\index.js:65:25
    at _fulfilled (C:\test\node_modules\bower\node_modules\q\q.js:834:54)
    at self.promiseDispatch.done (C:\test\node_modules\bower\node_modules\q\q.js:863:30)
    at Promise.promise.promiseDispatch (C:\test\node_modules\bower\node_modules\q\q.js:796:13)
    at C:\test\node_modules\bower\node_modules\q\q.js:604:44
    at runSingle (C:\test\node_modules\bower\node_modules\q\q.js:137:13)

$
$ npm i bower@1.7.1
...
$ node -e "require('bower').commands.list().on('end', function() {throw new Error('test');});"

$
$ npm i bower@latest
...
$ node -e "require('bower').commands.list().on('end', function() {throw new Error('test');});"

$

@sheerun sheerun closed this as completed in c2e0dc9 Apr 4, 2016
@sheerun
Copy link
Contributor

sheerun commented Apr 4, 2016

Thank you for reporting this. It's not fixes in master branch

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

Successfully merging a pull request may close this issue.

4 participants