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

Feature Suggestion: Allow Passing the Zbar Instance #19

Open
aaronclong opened this issue Mar 22, 2024 · 6 comments
Open

Feature Suggestion: Allow Passing the Zbar Instance #19

aaronclong opened this issue Mar 22, 2024 · 6 comments

Comments

@aaronclong
Copy link

aaronclong commented Mar 22, 2024

Overview


I really appreciate all the documentation put into the bundles of ZBar, but the process is quite difficult still. I was wondering. Would be possible to pass the Zbar Instance to the library.

The benefits of this would be great considering the additional complexity it would add. Namely, it would allow the application to decide how the WASM in bundled and deployed into the application. It would put less work in trying to work around the tooling failings in currents stacks.

Looking over the code, I noticed the instance function could probably be easily modified to accommodate this. Some factory function or user config would need to be assed to propagate to it here though.

export const getInstance = async (customLoader?: () => Promise<ZBarInstance>): Promise<ZBarInstance> => {
  if (customLoader) {
     return await customLoader();
  }
    
  return await zbarInstancePromise
}

Side node

For better encapsulation, you can refactor the zbarInstancePromise function to something like below (hard to type code in comments). This is pure suggestion and can be completely ignored.

let zbarInstance: ZBarInstance

const zbarInstancePromise = (() => {
  let zbarInstance: ZBarInstance

  const instancePromise = async () => {
    zbarInstance = await zbarJs()
    if (!zbarInstance) {
      throw Error('WASM was not loaded')
    }
    return zbarInstance
  };
  
  return instancePromise
})()
@undecaf
Copy link
Owner

undecaf commented Mar 26, 2024

Thank you @aaronclong for your proposal!

I agree very much that it is hard sometimes to make bundlers perform as intended, and that it may be even less cumbersome to write a custom loader for WASM content.

However, the purpose of the zbar-wasm library is to provide a complete barcode scanner for a variety of environments, which certainly makes the documentation more convoluted in several places.

Making library consumers responsible for WASM loading (even if only as an option) would contradict this purpose, making zbar-wasm act as a JavaScript facade for custom-loaded WASM code in those cases. Therefore I think that such an approach would be better handled in a dedicated project.

OTOH, any ideas for reducing/eliminating the amount of bundler tweaking that is required for WASM loading would be very much appreciated (if you wish to share).

@aaronclong
Copy link
Author

Thank you @aaronclong for your proposal!

I agree very much that it is hard sometimes to make bundlers perform as intended, and that it may be even less cumbersome to write a custom loader for WASM content.

However, the purpose of the zbar-wasm library is to provide a complete barcode scanner for a variety of environments, which certainly makes the documentation more convoluted in several places.

Making library consumers responsible for WASM loading (even if only as an option) would contradict this purpose, making zbar-wasm act as a JavaScript facade for custom-loaded WASM code in those cases. Therefore I think that such an approach would be better handled in a dedicated project.

OTOH, any ideas for reducing/eliminating the amount of bundler tweaking that is required for WASM loading would be very much appreciated (if you wish to share).

Thanks for responding to this issues. I appreciate it!

Unfortunately, I will try to keep an eye out for the bundler settings. However, it gets complicated with NextJs and other layer frameworks. So, I can't say for the moment if any tweaks would be useful at the moment.

Admittedly, I disagree with the line draw here, "Making ... consumers responsible for WASM loading (even if only as an option) would contradict this purpose". Since the user would still be getting the WASM from your library but bundling it another manner for their downstream. It is reminiscent of allow dynamic vs static linking OS binaries in my opinion. However, I respect that this is your decision.

If I find any magic to make this work for my setup, I will try to forwarded it.

@undecaf
Copy link
Owner

undecaf commented Apr 1, 2024

I am thinking of pre-processing the WASM file in the build process of zbar-wasm, converting it to a JS file that can be bundled without further ado. This would replace the current inline bundling option.

Please allow me some time for playing around with this.

@aaronclong
Copy link
Author

aaronclong commented Apr 2, 2024

I am thinking of pre-processing the WASM file in the build process of zbar-wasm, converting it to a JS file that can be bundled without further ado. This would replace the current inline bundling option.

Please allow me some time for playing around with this.

I don't fully understand what that would help with to be honest. I do think the standalone binary should be preserved. The inline javascript variants remove the incremental compilation aspect of WASM causing slower and more expensive loads.

To be clear, my problem was never with the WASM file directly but how the script referenced it. I did notice that the package.json exports of the WASM didn't work for whatever reason.

@undecaf
Copy link
Owner

undecaf commented May 22, 2024

@aaronclong Does this commit happen to solve your issue?

@aaronclong
Copy link
Author

@aaronclong Does this commit happen to solve your issue?

Hey! I am just skimming it now. It looks like it should work! I'm going to review more in depth later.

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