OriginProtocol / origin-js

We've moved to a monorepo: https://github.com/OriginProtocol/origin
MIT License
81 stars 33 forks source link

Optimize method saveDataURIAsFile #511

Open franckc opened 5 years ago

franckc commented 5 years ago

The method ipfsService.saveDataURIAsFile seems sub-optimal. I haven't had time to test thoroughly on Node and Browser but we should be able to replace it with something that does not involve converting every single byte into an Uint8Array. Something like this:

 async saveDataURIAsFile(dataURI) {
    let binary
    if (typeof Blob === 'undefined') {
      binary = new Buffer(dataURI.split(',')[1], 'base64')
    } else {
      const mimeString = dataURI
        .split(',')[0]
        .split(':')[1]
        .split(';')[0]
      const data = new Buffer(dataURI.split(',')[1], 'base64')
      binary = new Blob([data], {type: mimeString})
    }
    return await this.saveFile(binary)
  }

While doing that, we should add some unit test on the back-end for uploading Data URI. Here is a snippet for creating a Data URI for an image:

      const imageBin = fs.readFileSync(imagePath)
      const imageBase64 = new Buffer(imageBin).toString('base64')
      const contentType = imagePath.endsWith('jpg')
        ? 'image/jpeg'
        : 'image/png'
      const medium = {
        url: `data:${contentType};base64,${imageBase64}`,
        contentType: contentType
      }
tomlinton commented 5 years ago

I remember working on that and having to do some strange things to maintain both node and browser compatibility. And also maintain support across different browsers. I don't remember the exact details but I'd suspect what I've done was mostly for compatibility reasons. I'm sure it can be improved though!

franckc commented 5 years ago

I tested my suggestion on both Node and Chrome browser. But I agree more testing on different type of browsers would make me feel more confident...

I'm actually not clear on why we need to use Blob on browser... Why not passing a Buffer to the saveFile() method in all cases ?

tomlinton commented 5 years ago

I think browsers don't have Buffer?