added check for errors from headless #22

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

spalger commented Nov 22, 2013

headless currently does not catch errors, or send callbacks errors, but if kesla/node-headless#10 get's merged, or any other check for it's major dependency is put into place, this error arg might be useful for OSX users who have not installed Xvfb.

graue referenced this pull request in kesla/node-headless Apr 27, 2014

Merged

Added check for enoent error #10

graue commented Apr 27, 2014

kesla/node-headless#10 is now merged, so this PR has become relevant.

But if you haven't installed Xvfb, this hangs endlessly after printing the error. How about throwing instead of passing into the callback?

// instead of this...
cb(new Error('Attempted to launch ' + name + ' headlessly, ' +
    'but received error "' + err.message + '"'));

// do this
throw new Error('Attempted to launch ' + name + ' headlessly, ' +
    'but received error "' + err.message + '"');

spalger commented Apr 28, 2014

I disagree @graue. I think that standard conventions should be followed and errors should "bubble up". If a module using node-headless ignores the error, it needs to be modified.

Keep in mind that an error thrown here could not be caught by the caller, it must be passed back in order for the consumer to choose how to act.

graue commented Apr 29, 2014

s/node-headless/browser-launcher/, but yes you're absolutely right. Too many layers... I thought I was looking at the PR for testling when I posted that.

gsf referenced this pull request in substack/testling Jun 29, 2014

Open

Example testling test fails with unhandled error #66

spalger closed this May 9, 2017

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