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

Fixed exceptions thrown from the user's callback being silently swallowed #11

Conversation

hgwood
Copy link

@hgwood hgwood commented Aug 18, 2014

Example

ghpages.publish('dist', function (err) {
  if (err) throw err;
})

When err is not null:

  • Behavior in version 0.2.0: the exception is silently swallowed and the program finishes normally without any error or warning. The outcome is the same if the silent option is enabled.
  • Behavior with this PR: the exception is thrown and the program stops with the error message and stacktrace. If the silent option is enabled, the error message is Unspecified error (run without silent option for detail), as expected.

@MoOx
Copy link
Contributor

MoOx commented Apr 23, 2015

@tschaub can you merge an release this ? Should be nice to do so :)

@vvo
Copy link

vvo commented Dec 2, 2015

@tschaub do you need help maintaining gh-pages? I depend on this module and would like to avoid rewriting it/forking it.

Let me know I can help/triage issues and publish. Just add me to github and npm if you feel like, thx.

@tschaub
Copy link
Owner

tschaub commented Dec 2, 2015

It is not clear to me that calling the done callback with the done callback is the correct thing to do.

What needs to be done is to port the tests from grunt-gh-pages (see #3) so we can have more confidence that proposed changes are sensible. And then we could require a test for a change like this before merging.

@tschaub
Copy link
Owner

tschaub commented Dec 8, 2015

Updated error handling in #35, #36, and #37. [email protected] includes these updates. Please open new issues if you encounter swallowed or unhandled errors.

@tschaub tschaub closed this Dec 8, 2015
@hgwood
Copy link
Author

hgwood commented Dec 9, 2015

I'm sorry I was not reactive enough after your message 7 days ago.

Why do you doubt that calling Q's done is the correct thing to do? Q's documentation says:

The Golden Rule of done vs. then usage is: either return your promise to someone else, or if the chain ends with you, call done to terminate it. Terminating with catch is not sufficient because the catch handler may itself throw an error.

The case at hand precisely matches the second case mentioned in the doc because the promise created in publish is not returned to the caller.

Furthermore, the implementation in #37 still swallows the error and the stack trace. It handles the error fine (but that was already the case) and logs its message (that's the new part and it's a greatly appreciated improvement). However, an application cannot react to it and it doesn't blow up everything like a developer would expect. I think that's unfortunate.

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

Successfully merging this pull request may close these issues.

4 participants