baudehlo / node-phantom-simple

Simple bridge to phantomjs for Node
MIT License
201 stars 70 forks source link

page.evaluate(fn, args, cb) doesn't propagate error to callback #134

Closed raphaelokon closed 8 years ago

raphaelokon commented 8 years ago

Hi.

I noticed that error during the evaluation of a function insde the phantom are not propagated to the callback, eg.:

page.evaluate((arg) => {
   // Make something break inside the page
}, (err, result) => {
  if(err) //Handle the error – err is always null
});

Any ideas? Thanks.

Reewr commented 8 years ago

As far as I know, PhantomJS does not support catching errors through the evaluate function. I believe that the error sent to the callback is regarding errors that can happen with the connection between node-phantom-simple and phantomjs and not the execution of the function.

If you want to make sure that the code runs, you could attach a listener to page.onError which listens for JavaScript execution errors that occur in the browser instance that PhantomJS runs.

As a sidenote: Using arrow functions for the evaluate function (the first parameter) is not supported due to how it's stringified. PhantomJS does not support ES6 syntax and the function in your example is stringified to (arg) => {} (unless you happen to use an ES6 transpiler).

raphaelokon commented 8 years ago

Hi @Reewr.

Just saw your reply – thanks. I did just that after looking into the source: page.onError to propagate the error from Phantom to Node. Not optimal as I have to put my Promises into a weird race condition with page.evaluate and page.onError to get a safe result from page.evaluate.

Regarding your sidenote: The arrow func was just to keep stuff short and I was already using a normal function. Thanks for pointing out tho – I noticed that while skimming the source as well.

Reewr commented 8 years ago

@raphaelokon

As you saw, it can indeed be propagated this way. It should be noted that in order to set page.onError all you need to do is page.onError = (message, trace) => {};, as it does not need to be set through a callback. It does, however, only poll PhantomJS for errors every 500th millisecond, meaning there might be a large delay.

I don't know how your setup is and why you need page.onError, but you could always try wrapping the whole evaluate function in a try-catch and return an object that says whether an error occured or not.

page.evaluate(function() {
  try {
    // do something
    return {error: null}
  catch(error) {
    return {error: error.message, trace: error.trace}
  }
}, function(err, obj) {
  if (obj.error) {

  }
});

Not the most ideal of ways to do it, but it'll save using onError and also save you some time by not having to wait for node-phantom-simple to poll PhantomJS for events. All you need to worry about in the above case is that the return object is serializable (can be stringified).

raphaelokon commented 8 years ago

Cheers. I noticed the poll … and the try/catch would have been my next stab. Works great as the outer func is still promisified and fits into the main flow.