capture-frame icon indicating copy to clipboard operation
capture-frame copied to clipboard

Some feedback

Open jimmywarting opened this issue 7 years ago • 0 comments

Formats

I tested chrome, ff & safari on this page:

https://kangax.github.io/jstests/toDataUrl_mime_type_test/

Apparently

  • Safari supports png, jpeg, gif, bmp, and tiff
  • Chrome supports png, jpeg and webp
  • Firefox supports png, jpeg and bmp

(don't have IE/Edge on mac)

So i think this check should be removed and just allow whatever format gets thrown at it. Other browser & format will be added at some point. If it isn't supported then it will fallback to png. there is no harm in testing other unsupported formats.

https://github.com/feross/capture-frame/blob/947bb9ac0a0b03d0a418208666e1a324b027cc03/index.js#L18-L20

Since it will default to png then there is no need to check for format either

https://github.com/feross/capture-frame/blob/947bb9ac0a0b03d0a418208666e1a324b027cc03/index.js#L14-L16

since image/undefined will be a png anyway

But if you insist on keeping it, then use default parameter function n(format = png)

Output

Right now it returns a Buffer and makes it a dependent on Buffer. I think that it could as well return a Uint8Array instead. Uint8Array and Buffer are both instances of Uint8Array. That would make this package a lot smaller when using browserify. Node are now also able to write any TypedArray with fs and are not restricted to only Buffer. So there is that...

Using canvas.toBlob i think is also better. Which means it will be async... So returning a new promise would be something worth considering?

then you can use the FileReader or Response to turn a blob into a arrayBuffer and then resolve the promise with a Uint8Array

// Response
new Promise(rs => 
  canvas.toBlob(blob => {
    rs(new Response(blob).arrayBuffer().then(ab => new Uint8Array(ab)))
  }, 'image/' + format)
)
// FileReader
new Promise(rs => 
  canvas.toBlob(blob => {
    const reader = new FileReader()
    reader.onload = () => {
      rs(new Uint8Array(reader.result))
    }
    reader.readAsArrayBuffer(blob)
  }, 'image/' + format)
)

But i almost think it should return a Blob instead of a Buffer or TypedArray. then you are able to check if the format was what you wished for by checking if the blob.type matches. You are then also able to create a ObjectURL or append the blob to a FormData. Wish I think could be more useful. it's also more RAM friendlier to have a blob that might point to some place on the HDD instead of having the hole buffer in the RAM. ...There is no easy way to guess the mime type of a buffer and do the opposite thing.

If I used webp in firefox it would return a png (with buffer). So if i assumed it worked and save it as .webp then it would have the wrong extension type

Quality

Think you missing the quality parameter as well... 0-1 (default is 0.8)

jimmywarting avatar Oct 07 '18 21:10 jimmywarting