brenden / node-webshot

Easy website screenshots in Node.js
2.12k stars 285 forks source link

Getting a stream event to fire when I don't think it should be? #94

Open ralucas opened 9 years ago

ralucas commented 9 years ago

Hi, I'm getting a stream event to fire when I don't think it should be:

app.post('/renderUrl', urlEncodedParser, function(req, res) {
  var url = req.body.url;
  var filepath = '/path/to/file.png';

  var options = {
    siteType: 'url',
    streamType: 'png',
    errorIfStatusIsNot200: true
  };

  webshot(url, options, function(err, renderStream) {
    if (err) {
      if (/Status must be 200/.test(err.message)) {
        res.send('urlFailed'); //this is firing on a 404
      } else {
        res.send(err);
      }

    } else {
      var file = fs.createWriteStream(path.join(__dirname, filepath), {encoding: 'binary'});

      renderStream.on('data', function(data) {
        file.write(data.toString('base64'), 'binary');
      });

      renderStream.on('end', function(dt) {
        console.log('in the renderstream end'); //This console.log is firing on a 404
        fs.readFile(path.join(__dirname, filepath), function(err, data) {
          if (err) {
            res.send(err);
          } else {
            //these events are firing on a 404
            res.send(data);
          }
        });
      });
    }
  });
});

Am I thinking about this incorrectly or something bad here in the code?

Thank you for any help.

Cheers!

Richard

troyfendall commented 9 years ago

I had this problem as well. Unfortunately I couldn't find a good solution that allowed me to use a stream and have webshot throw an error for a non-200 result. I found that it does throw an error if you write the webshot to a file rather than using a stream, but if you ultimately want to work with a stream, writing it to a temp file then opening a read stream to that file is suboptimal to say the least. It would be nice if webshot could recognize a non-200 error and throw the error itself, or at least take an http stream as an alternate input since other http libraries throw an error on 4* and 5* results.

My solution, since I'm in control of the webpage being rendered, and I know that it's fast and little content, is that I wrap the call to webshot in an up-front http request, which will throw the error I need before I initiate webshot.

Not sure if that helps, but I figured I'd +1 the issue.

msurdi commented 8 years ago

Looking at this line, seems like if the subprocess outputs to stdout an error, the only possibility considered for it is to be a js error, when the truth seems to be that for non 200 errors, the process also writes to stderr.

To me, it looks like that code should read something more like:

phantomProc.stderr.on('data', function(data) {
  if (options.errorIfJSException || options.errorIfStatusIsNot200) {
    s.emit('error', ''+data);
  }
});

for now, my workaround has been to enable errorIfJSException and do in my own code something like:

let stream = webshot(.........)
stream.on("data", /*happy path*/)
stream.on("error", /*error path*/)

keeping in mind that the happy path will be always called, but in the case of an error the error path will have been called before so you can set a flag or something.