Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs suggestion: clarify formats only supported in "pure" mode #140

Open
getify opened this issue Dec 24, 2024 · 5 comments
Open

docs suggestion: clarify formats only supported in "pure" mode #140

getify opened this issue Dec 24, 2024 · 5 comments

Comments

@getify
Copy link

getify commented Dec 24, 2024

As I have been looking into this library, and the underlying zxing-wasm it uses... I've discovered that formats like "rm_qr_code" are extensions provided only in the "pure" mode, but are not supported in the polyfill (since the browser doesn't natively support them... yet at least).

I was hoping maybe you'd consider updating the README docs to clarify this point, saving others the time it took me to figure this out.

@Sec-ant
Copy link
Owner

Sec-ant commented Dec 25, 2024

Yes, only pure rm_qr_code can be reliably detected, you're right, I should update the README to reflect this point.

but are not supported in the polyfill (since the browser doesn't natively support them... yet at least).

I'm not sure I understand it, this polyfill should support detecting pure rm_qr_code, if it does not, it's a bug.

(This polyfill will not be activated if it finds a global BarcodeDetector, maybe that's why it doesn't work for you. You can import the detector explicitly to try out the detector implemented by this library.)

@getify
Copy link
Author

getify commented Dec 26, 2024

What I mean is, the "rm_qr_code" detection type is only assured to work if you import the library in the "pure" way (aka, a "ponyfill").

If you import the polyfill, and the browser already has the API, then the polyfill doesn't add this "rm_qr_code" detection type on top of it. It just gets out of the way, which would then cause an attempt to use the "rm_qr_code" scan mode to fail.


As a side note, I think this library's current design/behavior is a bit problematic, that I would suggest you reconsider.

NOTE: Before I go on... an assumption: I searched and didn't find any indication that these modes are under future consideration by the spec bodies, to be added. If that's actually happening, and I wasn't aware, please point me to that information, and then disregard the rest of this message as it's inapplicable.

The spirit of polyfills is that they're supposed to provide the exact same capabilities (no more, no less) as the native API they're forward-replacing... such that once the native API ships in a browser/environment, the polyfill silently skips loading, and the app's code should keep working the same.

By adding the extra scanning modes to the polyfill, you are creating this situation where developers might, unknowingly, import the polyfill, and expect behavior to work the same for browsers with the native API as for browsers without, but if they happen to accidentally rely on a non-standard feature (like these extended scan modes), then their app code could break when a user's browser updates to add this API, even if the app code itself didn't change at all. This is certainly not desired by either users or developers, which is why the idea behind polyfills is always to only replace actual standardized behaviors/capabilities and not to add additional stuff.

My suggestion would be, that the ponyfill ("pure") implementation is the only form that these extra scanning modes should be supported. If someone uses the polyfill import form, and their browser DOES NOT have the native API yet... and they try to use one of the non-standard scanning modes, I think that should throw an error.

I think that's more consistent without how software typically organizes itself between a polyfill (standard behavior only) and any extensions they want to additionally provide in a pure-imported ponyfill.

@Sec-ant
Copy link
Owner

Sec-ant commented Dec 29, 2024

What I mean is, the "rm_qr_code" detection type is only assured to work if you import the library in the "pure" way (aka, a "ponyfill").

If you import the polyfill, and the browser already has the API, then the polyfill doesn't add this "rm_qr_code" detection type on top of it. It just gets out of the way, which would then cause an attempt to use the "rm_qr_code" scan mode to fail.

Ah I see, that's because if the Barcode Detection API is already implemented by the browser, the side-effects version of this package will silently opt out, without checking if the capability is fully provided by the browser, as it only checks the existance of globalThis.BarcodeDetector.

I see at least one point how I should improve this package, that is replace pure and side-effects with ponyfill and polyfill for less ambiguity when disussing things 😄.

My suggestion would be, that the ponyfill ("pure") implementation is the only form that these extra scanning modes should be supported. If someone uses the polyfill import form, and their browser DOES NOT have the native API yet... and they try to use one of the non-standard scanning modes, I think that should throw an error.

I see it differently, as none of the supported barcode formats are considered standard according to the spec (and even the spec is a draft version) but are platform dependent:

The list of supported BarcodeFormats is platform dependent, some examples are the ones supported by Google Play Services and Apple’s QICRCodeFeature.

So it's really difficult or impossible for a library to provide a polyfill with different native supported barcode formats for different platforms (and probably) in different versions on different OSes. That's why a user is suggested to use the getSupportedFormats in advance before actually detecting any barcodes.

Also, I believe some versions of Chromium on macOS don't support blob inputs for detection. They don't even handle all the cases exactly the same as how the draft spec instructs. I consider these as implemetation bugs and also fixed them in this polyfill.

Apart from that, I agree with you that polyfills should try to be as consistent as possible with the native implementation. For example, I tried my best to keep all the error messages and DOM exceptions the same with the Chromium implementation. Speaking of this, I admit I didn't bother to check how other browsers handle the errors. 😉

NOTE: Before I go on... an assumption: I searched and didn't find any indication that these modes are under future consideration by the spec bodies, to be added. If that's actually happening, and I wasn't aware, please point me to that information, and then disregard the rest of this message as it's inapplicable.

I don't see these barcode formats as under future consideration as I think they are fine to be provided as how they're now. But I do am planning a new version as the underlying zxing-wasm package is about to release v2, and I also see room for improvement on the documetation and subpath exports namings now that you opened this issue to bring about some confusing behaviors I wasn't aware of.

Anyway, thanks for your suggestions.

@getify
Copy link
Author

getify commented Dec 29, 2024

That's why a user is suggested to use the getSupportedFormats in advance before actually detecting any barcodes.

This is a good point.

But even in using that, to avoid unexpected exceptions being thrown, I still see it as a footgun gotcha -- from a docs perspective! -- that if I were to use the "side effects" (aka "polyfill") version of the import, I would have a case where some users' browsers would have the "rm-qr-code" capability, and others would not, even though that capability is advertised as supported by the library (and is indeed not supported by any browsers, as far as I know).

I mean, clearly, the solution is, only use the "pure" (aka "ponyfill") import mode. Sure.

But I guess what I'm saying is, the docs don't make it clear that if you choose the polyfill, you don't have the guarantees of support, you only get those guarantees if you use the ponyfill.

If there were some guarantees of scan types the native API provides, and a dev only needs one/some of those... then I could see how they'd want to use the polyfill, to allow the native API to work (if present) and save the downloaded bytes of the full library. But if there's truly no spec-guaranteed formats at all, then as a dev you're sort of gambling on if users will be able to scan their intended barcodes or not. That sort of implies that maybe there's not really ever a case where a developer would rightly want the polyfill.

Certainly, the docs could do those developers a favor and make it super clear and obvious that none of the scan formats are guaranteed if you choose the polyfill import mode. That should be a big red warning light to them that they should be really careful in choosing the polyfill.

Thanks again for the help and clarifications here!

@Sec-ant
Copy link
Owner

Sec-ant commented Dec 29, 2024

If there were some guarantees of scan types the native API provides, and a dev only needs one/some of those... then I could see how they'd want to use the polyfill, to allow the native API to work (if present) and save the downloaded bytes of the full library.

Strictly speaking, there's no guarantees per the spec. But I believe at least qr_code is one of the de facto barcode formats that are supported in all the platforms which implement this API natively. And although I'm also an advocator of using the ponyfill whenever one can, there're real cases where developers would like to use a polyfill version for their customers. So I still see the value of providing it. Also I see removing it as too much a breaking change.

Certainly, the docs could do those developers a favor and make it super clear and obvious that none of the scan formats are guaranteed if you choose the polyfill import mode. That should be a big red warning light to them that they should be really careful in choosing the polyfill.

Yes, I already took your suggestions and started preparing for the next release with a big update of the README document: https://github.com/Sec-ant/barcode-detector/tree/breaking/v3?tab=readme-ov-file#polyfill

I really appreciate your input on this issue. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants