Trouble with base64 encoded buffers via browserified ipfsApi

[ckeenan]

Looking up a base64 encoded buffer works fine with the node ipfs-api module. In the browser, it appears there's a problem converting it to a string from Uint8Array. Here is some code that encounters this

var gifData = 'R0lGODlhAQABAIAAAAUEBAAAACwAAAAAAQABAAACAkQBADs=';

ipfsApi.add(new Buffer(gifData, 'base64'), function(err, ipfsArr) {
    if (err) throw err;

    ipfsApi.cat(ipfsArr[0].Hash, function(err, blobStr) {
      if (err) throw err;
      console.log('equal:', Buffer(blobStr, 'ascii').toString('base64') === gifData);   // false
    });
});

This change -- [https://github.com/ckeenan/node-ipfs-api/commit/0a9290a17e970b8a447cd0ba4b3a202693a1dcb5] -- resolves it, but unsure if best solution

[jbenet]

i think this is related to go-ipfs treatement of the buffers maybe. see ipfs/kubo#1582

[ckeenan]

That issue does affects this if there's going to be a change in the response format of ipfs cat.. but I think there's an unrelated issue here:

Running ipfsApi.cat, the http request for ipfs cat responds with a buffer. The browserified version appends the buffer's string Uint8Array.prototype.toString() to '' which sometimes returns an incorrectly encoded string -- https://github.com/ipfs/node-ipfs-api/blob/master/src/request-api.js#L58. If buffers are the correct response type for api call to ipfs cat, the expected result could be achieved with Buffer.concat() or the Uint8Array can also be appened to '' as a binary string.

Since ipfsApi.add expects a Buffer, seems reasonable for ipfsApi.cat to return Buffers? In which case maybe Buffer.concat() is the way to go

[jbenet]

thoughts @diasdavid @victorbjelkholm @dignifiedquire @bcomnes ?

[daviddias]

@ckeenan you definitely have a point, looking at that func it is hard to understand the reason on why that call. (+ things like https://github.com/ipfs/node-ipfs-api/blob/master/src/request-api.js#L77 and https://github.com/ipfs/node-ipfs-api/blob/master/src/request-api.js#L83-L84 suggest that we need to revisit it)

Now with the new documentation that @victorbjelkholm is writing (#58), we should (or even must :)) write tests accordingly with the expectations of function calls to catch this cases.

[daviddias]

@ckeenan we just added a considerable amount of tests to node-ipfs-api (#81) and landed the field for browser tests. Could you verify that the problem remains and if so, could you add a test case demonstrating the issue, so it is easier to target the fix? Thank you :)

[dignifiedquire]

@ckeenan ping

[ckeenan]

@dignifiedquire ACK. I am trying to test with a simple html file that includes dist/ipfsapi.js as a <script />. ipfs.cat returns a stream and not sure I can parse that in-browser. Is this expected?

[dignifiedquire]

Yes this is expected, you can easily parse it like this:

ipfs.cat('some hash', function (err, stream) {
  var res = ''

  stream.on('data', function (chunk) {
    res += chunk.toString()
  })

  stream.on('error', function (err) {
    console.error('Oh nooo', err)    
  })

  stream.on('end', function () {
    console.log('Got:', res)
  })
})

[ckeenan]

Oh excellent. So this lets the application concatenate the data any way it likes. That indeed fixes this issue where appending the stream's data arrays to an empty string can result in incorrect encodings for some types of buffers. Looks like this is ready to close @dignifiedquire

[dignifiedquire]

@ckeenan great to hear :)