edi9999 / jsqrcode

[deprecated] Lazarsoft's jsqrcode as a node module, object oriented, and with tests
Apache License 2.0
280 stars 63 forks source link

Canvas image data api fixes and documentation #11

Closed TCMiranda closed 8 years ago

TCMiranda commented 8 years ago

The readme contains the changes I made. I appreciate any considerations Regards; Tiago.

edi9999 commented 8 years ago

Hi, I also like this library, but yes the interface is not that well designed.

I think the reason for your change is good, of course the error should come out even when using web workers.

However, your changes break the CI, it seems that it breaks backward compatibility. Can you have a look.

TCMiranda commented 8 years ago

Hi @edi9999 I will give it a look... Thanks!

TCMiranda commented 8 years ago

@edi9999 the test broke because of the decode({ width, height }, data) syntax which I "fixed"

Since the canvas context.getImageData(0, 0, width, height) returns an ImageData object, like { data: Uint8ClampedArray, width: Int, height: Int }, the right thing to do, in my opinion, should be require that kind of object format, only at the first parameter:

decoder.decode(ImageData)

That way, the api would be more generic, accepting:

decoder.decode('' | ImageData | URL | DataURL)

Is it a smart change? I think that since the .decode({ width, height }, data) is broken (does not throws errors), is ok to change the way it's called.

Any way.. If that is true, the second parameter would be the callback, as usual.

Waiting for your opinion.

Thanks!!

edi9999 commented 8 years ago

I think what you propose makes sense.

However, since this library doesn't get updated very often, I don't like the idea of doing a breaking change.

Maybe what we could do would be to accept both types of arguments, either two arguments or only one.

When we decide to hit a new release version, we then drop support for multiple arguments.

Does it make sense ?

TCMiranda commented 8 years ago

@edi9999 Thanks, of course it does.

I did some changes in order to maintain the compatibility.

Also, i've added another test, with the imageData format support.

Regards

edi9999 commented 8 years ago

Thanks, merged