danwrong / restler

REST client library for node.js
MIT License
1.99k stars 389 forks source link

Fatal error: Second argument needs to be a buffer #250

Open kundaiAtPropertyMe opened 7 years ago

kundaiAtPropertyMe commented 7 years ago

Since I upgrade node.js version to the latest (8.0), when I use rest.post(url, options) to post a sourcemap file (json), it returns error Fatal error: Second argument needs to be a buffer

kundaiAtPropertyMe commented 7 years ago

since fs.read is changed by node and older function is removed.

The way to resolve this bug is modify the multipartform.js file

change the write function

write: function(stream, callback) {

var self = this;

  //first write the Content-Disposition
  stream.write(this.header());

//Now write out the body of the Part
if (this.value instanceof File) {
  fs.open(this.value.path, 'r+', function (err, fd) {
      var stats = fs.fstatSync(fd);
      var bufferSize = stats.size
      if (err) throw err; 
      var chunkSize = 512;
      var position = 0;
      var buf = new Buffer(bufferSize);
    (function reader () {

        if ((position + chunkSize) > bufferSize) {
            chunkSize = (bufferSize - position);
        }

      fs.read(fd, buf, position, chunkSize, position,  function (er, chunk, buf) {
        if (er) callback(err);
        stream.write(buf.slice(position, chunkSize + position));
        position += chunkSize;
        if (position < bufferSize) reader();
        else {
              stream.write("\r\n")
              callback();
              fs.close(fd);
            }
      }); 
    })(); // reader() 
  });
 } else if (this.value instanceof Data) {
  stream.write(this.value.data);
  stream.write("\r\n");
  callback();
 } else {
  stream.write(this.value + "\r\n");
  callback();
}

}

kundaiAtPropertyMe commented 7 years ago

create fork and send pull request about this change https://github.com/danwrong/restler/pull/251

tkristensen commented 6 years ago

+1 on merging this, I'd like to use restler in Node LTS builds but can't without this error resolved.

reco commented 6 years ago

+1 yes please merge. is there a reason not to?