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

Allow to provide URL to zxing-wasm #354

Closed
raimund-schluessler opened this issue Aug 6, 2023 · 11 comments
Closed

Allow to provide URL to zxing-wasm #354

raimund-schluessler opened this issue Aug 6, 2023 · 11 comments

Comments

@raimund-schluessler
Copy link

Since [email protected] the library uses @sec-ant/barcode-detector. This in turn loads a @sec-ant/zxing-wasm web assembly file via a HTTP request to a third-party server. This is problematic for servers with a strict CSP policy preventing such requests.

I see that @sec-ant/barcode-detector allows to set a custom URL to load the wasm file from, via setZXingModuleOverrides, see https://github.com/Sec-ant/barcode-detector#setzxingmoduleoverrides.

Is there a way to use vue-qrcode-reader and set the custom URL for zxing?

@gruhn
Copy link
Owner

gruhn commented Aug 6, 2023

Good catch, thanks 👍 This is very annoying. Users shouldn't have to deal with this. There's a similar issue with providing web workers in packages. Ideally you would just bundle the wasm file with the package and somehow "include" it opaquely. But I don't know if there is an easy way to do that.

cc @dargmuesli @Sec-ant

@Sec-ant
Copy link
Contributor

Sec-ant commented Aug 6, 2023

Yep I didn't bundle the wasm file inside @sec-ant/zxing-wasm or @sec-ant/barcode-detector libs because the inline base64 data url would make the packages size very huge. I cannot provide a runtime fallback option either, because that also means I have to include the base64 url in some branch of the code. So I provided a default, relatively short and common cdn location, and exposed a function to let the users decide where to load their wasm files if they want to change this behavior.

If vue-qrcode-reader wants to hide this option away from the users and to not use a third-party server, the easy way would be downloading the wasm file from the CDN (version pinning is recommended), saving it somewhere in the project considered as assets, and importing it as a base64 url with Vite's asset importing query strings.

A small demo looks like this:

import wasmFile from "../public/zxing_reader.wasm?url";
import { setZXingModuleOverrides } from "@sec-ant/barcode-detector";

setZXingModuleOverrides({
  locateFile: (path, prefix) => {
    if (path.endsWith(".wasm")) {
      return wasmFile;
    }
    return prefix + path;
  },
});

const barcodeDetector: BarcodeDetector = new BarcodeDetector({
  formats: ["qr_code"],
});

fetch(
  "https://api.qrserver.com/v1/create-qr-code/?size=150x150&data=Hello%20world!"
)
  .then((resp) => resp.blob())
  .then((imageFile) => barcodeDetector.detect(imageFile))
  .then(console.log);

However, I suggest reexporting this setZXingModuleOverrides function or encapsulating it as a simpler form to let the users have the ability to control where they want to serve the wasm file may be a better option. Additionally, you can also keep the package size small.

PS:
You may notice I have updated @sec-ant/zxing-wasm and @sec-ant/barcode-detector just now. That's because when I was testing, I realized I put @types/emscripten in the dev depencies, so the argument of setZXingModuleOverrides is not properly typed. The recent version updates (@sec-ant/[email protected] and @sec-ant/[email protected]) fixed this problem.

@raimund-schluessler
Copy link
Author

@gruhn @Sec-ant Thanks for taking care and the extensive explanation and example code. I think exposing the setZXingModuleOverrides function would be the best option, as it keeps the package size small and allows for the highest flexibility.

If I see it correctly, the wasm files are available as GitHub releases from https://github.com/Sec-ant/zxing-wasm-build/releases. So maybe it would even be possible to configure vite/webpack to handle the downloading automatically, somehow, for people that don't want to use the CDN, but their custom server.

@Sec-ant
Copy link
Contributor

Sec-ant commented Aug 7, 2023

@raimund-schluessler Those wasm files are automatically built by GitHub actions, they are kept to sync with the upstream ZXing Cpp source code. That means not all of the builds are consumed in the downstream @sec-ant/zxing-wasm and they don't have a clear version mapping. So downloading those releases is a bad choice (If the version is mismatched, the wasm build may use a different api from what the js code expected). Actually the whole building and consuming process is somewhat amateur because I wasn't expecting other people will use this package 😂. Maybe I should use stricter version rules, content-hash filenames and some more powerful build tools.

Back to the subject, actually, you don't have to manually download the wasm file. The package manager should've already downloaded them for you when you install any of the downstream packages, e.g. vue-qrcode-reader. You should find the file under this path: node_modules/@sec-ant/zxing-wasm/dist/reader/zxing_reader.wasm. So you can import it like this and no longer worry about wrong versions:

import wasmFile from "../node_modules/@sec-ant/zxing-wasm/dist/reader/zxing_reader.wasm?url";

gruhn added a commit that referenced this issue Aug 7, 2023
The BarcodeDetector polyfill fetches a WASM file at runtime from a
CDN. That's a problem for offline applications or applications that
run in a network with strict CSP policy. As a workaround the polyfill
exports a function to configure the WASM file URL. So we re-export
the function here.

Also, by default the BarcodeDetector polyfill installs itself by setting
`globalThis.BarcodeDetector` but only if there is no native support.
This is likely not SSR compatible. Also, native support `BarcodeDetector`
is inconsistent. For example, Chrome on MacOS does support `BarcodeDetector`
but does not support `Blob` inputs, violating the specification. To have
a more consistent support we simply always use the polyfill on all
platforms. That likely comes as at a performance cost in raw scanning speed
on supporting platforms but the download penalty is paid either way.

See #354 #353
@gruhn
Copy link
Owner

gruhn commented Aug 7, 2023

@Sec-ant Thanks a lot for the insights. I followed your suggestions and prepared a PR with updated packages and setZXingModuleOverrides re-export. Still gotta do some testing.

gruhn added a commit that referenced this issue Aug 7, 2023
The BarcodeDetector polyfill fetches a WASM file at runtime from a
CDN. That's a problem for offline applications or applications that
run in a network with strict CSP policy. As a workaround the polyfill
exports a function to configure the WASM file URL. So we re-export
the function here.

Also, by default the BarcodeDetector polyfill installs itself by setting
`globalThis.BarcodeDetector` but only if there is no native support.
This is likely not SSR compatible. Also, native support `BarcodeDetector`
is inconsistent. For example, Chrome on MacOS does support `BarcodeDetector`
but does not support `Blob` inputs, violating the specification. To have
a more consistent support we simply always use the polyfill on all
platforms. That likely comes as at a performance cost in raw scanning speed
on supporting platforms but the download penalty is paid either way.

See #354 #353
gruhn added a commit that referenced this issue Aug 8, 2023
The BarcodeDetector polyfill fetches a WASM file at runtime from a
CDN. That's a problem for offline applications or applications that
run in a network with strict CSP policy. As a workaround the polyfill
exports a function to configure the WASM file URL. So we re-export
the function here.

Also, by default the BarcodeDetector polyfill installs itself by setting
`globalThis.BarcodeDetector` but only if there is no native support.
This is likely not SSR compatible. Also, native support `BarcodeDetector`
is inconsistent. For example, Chrome on MacOS does support `BarcodeDetector`
but does not support `Blob` inputs, violating the specification. To have
a more consistent support we simply always use the polyfill on all
platforms. That likely comes as at a performance cost in raw scanning speed
on supporting platforms but the download penalty is paid either way.

See #354 #353
gruhn added a commit that referenced this issue Aug 8, 2023
The BarcodeDetector polyfill fetches a WASM file at runtime from a
CDN. That's a problem for offline applications or applications that
run in a network with strict CSP policy. As a workaround the polyfill
exports a function to configure the WASM file URL. So we re-export
the function here.

Also, by default the BarcodeDetector polyfill installs itself by setting
`globalThis.BarcodeDetector` but only if there is no native support.
This is likely not SSR compatible. Also, native support `BarcodeDetector`
is inconsistent. For example, Chrome on MacOS does support `BarcodeDetector`
but does not support `Blob` inputs, violating the specification. To have
a more consistent support we simply always use the polyfill on all
platforms. That likely comes as at a performance cost in raw scanning speed
on supporting platforms but the download penalty is paid either way.

See #354 #353
@gruhn
Copy link
Owner

gruhn commented Aug 8, 2023

@gruhn gruhn closed this as completed Aug 8, 2023
@raimund-schluessler
Copy link
Author

@gruhn @Sec-ant Thanks a lot for your effort. It works nicely now.

With a strict CSP enabled, one just needs to adjust it to allow wasm-unsafe-eval.

@raimund-schluessler
Copy link
Author

Just a little addendum how to copy the required wasm file to the output directory with webpack:

//webpack.config.js
const CopyPlugin = require("copy-webpack-plugin")

webpackConfig.plugins.push(
	new CopyPlugin({
		patterns: [
		  { from: "node_modules/@sec-ant/zxing-wasm/dist/reader/zxing_reader.wasm", to: "zxing_reader.wasm" },
		],
	}),
)

@Sec-ant
Copy link
Contributor

Sec-ant commented Sep 16, 2023

I think there's an underlying problem regarding the versions of vue-qrcode-reader, and the zxing_reader.wasm file (from the package @sec-ant/zxing-wasm):

vue-qrcode-reader doesn't pin the version of @sec-ant/barcode-detector in its deps list, which could result in users having a newer version (with minor/patch updates because of the ^ caret sign) of @sec-ant/barcode-detector and @sec-ant/zxing-wasm in their node_modules folder.

As vue-qrcode-reader is published with everything bundled, including the js glue code (the part of code that interoperates with the .wasm binary file) from @sec-ant/zxing-wasm. As a result, if a user wants to host the zxing_reader.wasm himself/herself and copies it from the node_modules folder, the user may end up hosting a mismatched newer version of the .wasm file, because the js glue code is hardcoded in the vue-qrcode-reader dist bundle and therefore is immune from any updates inside node_modules.

I don't think this is an issue of vue-qrcode-reader, because "bundle and publish” is actually quite the common way to publish a package in the js world. And package managers + semver should have already taken into account of the breaking changes for us appropriately.

But wasm is still a rather foreign object for most front end tooling, applications, and most importantly, for me. So I'm not very confident that I can always mark the breaking changes correctly in each update of @sec-ant/zxing-wasm. And I cannot be sure the .wasm and js glue code from different versions can work together even if there're only "minor" or "patch" differences between them.

So, what I want to suggest here is that:

  • For package users: Until we find a better way to address this issue, if you want to host the zxing_reader.wasm file yourself, make sure you're using the correct version that matches with the js glue code bundled inside vue-qrcode-reader. You can search zxing-wasm inside the dist folder of vue-qrcode-reader in your node_modules to double check.

    search "zxing-wasm" to double check the version of the js glue code
  • For @gruhn : Maybe we should pin the version of barcode-detector in the package.json of vue-qrcode-reader? I have just pinned the version of @sec-ant/zxing-wasm in the package.json of barcode-detector. So if barcode-detector is also pinned by vue-qrcode-reader, then each version of vue-qrcode-reader will only correspond with a single exact version of @sec-ant/zxing-wasm.

  • For everyone: Is there a better, less error-prone way to solve this issue? I'm all ears. 👂🙏

@dargmuesli
Copy link
Contributor

Pinning dependencies rocks

@gruhn
Copy link
Owner

gruhn commented Sep 16, 2023

Sounds good 👍

gruhn pushed a commit that referenced this issue Sep 17, 2023
use exact version rather than semver range to prevent users from
unexpectedly hosting a wrong `.wasm` file from `node_modules`
see: #354 (comment)
dargmuesli added a commit to maevsi/maevsi that referenced this issue Sep 17, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [vue-qrcode-reader](https://vue-qrcode-reader.netlify.app)
([source](https://togithub.com/gruhn/vue-qrcode-reader)) | [`5.3.4` ->
`5.3.5`](https://renovatebot.com/diffs/npm/vue-qrcode-reader/5.3.4/5.3.5)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/vue-qrcode-reader/5.3.5?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/vue-qrcode-reader/5.3.5?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/vue-qrcode-reader/5.3.4/5.3.5?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/vue-qrcode-reader/5.3.4/5.3.5?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>gruhn/vue-qrcode-reader (vue-qrcode-reader)</summary>

###
[`v5.3.5`](https://togithub.com/gruhn/vue-qrcode-reader/releases/tag/v5.3.5)

[Compare
Source](https://togithub.com/gruhn/vue-qrcode-reader/compare/v5.3.4...v5.3.5)

##### Bug Fixes

- **wasm:** pin `barcode-detector`
([be19c85](https://togithub.com/gruhn/vue-qrcode-reader/commit/be19c8553c8b59907adf154d430a45be8b631970)),
closes
[/github.com/gruhn/vue-qrcode-reader/issues/354#issuecomment-1722257982](https://togithub.com//github.com/gruhn/vue-qrcode-reader/issues/354/issues/issuecomment-1722257982)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/maevsi/maevsi).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi44My4wIiwidXBkYXRlZEluVmVyIjoiMzYuODMuMCIsInRhcmdldEJyYW5jaCI6Im1hc3RlciJ9-->
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

4 participants