GoogleChromeLabs / web-gphoto2

Running gPhoto2 to control DSLR cameras over USB on the Web
https://web-gphoto2.rreverser.com/
GNU Lesser General Public License v2.1
101 stars 16 forks source link

Create NPM package #7

Closed icheered closed 1 year ago

icheered commented 1 year ago

This project is awesome so I did my best to turn it into an NPM package so that it can be installed easily. A few things to note:

google-cla[bot] commented 1 year ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

RReverser commented 1 year ago

Nice, thanks!

Some thoughts:

icheered commented 1 year ago

@RReverser I've made an attempt at a wrapper class named 'Camera' that automatically manages the connection.schedule calls, making it easier for users to interact with the library. The class also incorporates a conditional import from libapi.mjs, ensuring that the library is only loaded within a browser environment. This provides the added advantage of saving the user from implementing ~20 lines of extra code themselves.

During my initial tests, I've verified that the following functionalities operate as expected:

Although I haven't extensively tested camera.capturePreviewAsBlob due to the complexity of rendering the video, initial indications show that the function behaves as intended and returns the appropriate data.

As for automating package releases, I'll certainly explore the possibilities. However, considering that the repository's codebase isn't likely to undergo frequent updates, the necessity for automating the process seems somewhat diminished. Manual publication of a new release is a straightforward command (`npm publish') away. Additionally I'm aware that storing build artifacts in the repository isn't a widely favored approach. However, given the relatively small size of these files, I believe the current setup presents an acceptable compromise without requiring elaborate workarounds.

Feel free to make changes to the code in the PR before merging, I'm note particularly well-versed in writing these kinds of wrapper classes and I wouldn't be surprised if I made some mistakes or missed easy opportunities for improvement.

RReverser commented 1 year ago

However, considering that the repository's codebase isn't likely to undergo frequent updates, the necessity for automating the process seems somewhat diminished.

Yeah that's fair. Automation could in theory make it easier to update libgphoto2 automatically in the future as well, but for that the changed would need to be backported in a PR first. Maybe something to leave for the future.

If we do keep it in the repo, could you at least put built artifacts into a separate library (e.g. dist) and explicitly exclude it from Github diffs via https://docs.github.com/en/repositories/working-with-files/managing-files/customizing-how-changed-files-appear-on-github.

icheered commented 1 year ago

I moved your demo into a separate examples folder as I intend to add a Sveltekit example in there as well once the package is on NPM :)

icheered commented 1 year ago

@RReverser I have made some more minor changes and I'm almost done with the Svelte demo but I want to make a separate PR for that. I also confirmed that video is working as expected with the new implementation!

I think its ready for publishing :)

RReverser commented 1 year ago

Some final changes needed, but otherwise this is shaping up nicely, thanks :)

icheered commented 1 year ago

@RReverser Implemented your feedback!

RReverser commented 1 year ago

Hm my follow-up comment disappeared for some reason, but please fix ModulePromise as requested earlier - it needs to be a singleton stored outside of Camera class, yes, to avoid recompiling / re-instantiating same Wasm module on each camera instance.

Browser caching makes it a bit less of a problem but it's still not great to recreate the whole "world" for camera instantiation.

icheered commented 1 year ago

Alright I added the types.d.ts file with a class definition for Camera and removed the jsdoc syntax from camera.js. Now the types are automatically inferred 🎉. I made Camera a named export instead of a default export since this is needed for the automatic inference. I also moved ModulePromise outside the Camera class.

RReverser commented 1 year ago

Thanks, I think this is looking good! I'll look into updating the official demo (aka what used to be the main app).

icheered commented 1 year ago

Awesome, thanks @RReverser! Could you also publish the package to NPM using npm publish? That way I can immediately use it for a project I'm working on :)

RReverser commented 1 year ago

I'm trying it out locally now and there are some more minor updates I need to do before publishing, but hopefully will do those soon.

RReverser commented 1 year ago

Hm I don't see anything relevant that was changed by this PR, but looks like ./build.sh started to fail, both locally and on CI:

checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables... 
checking whether we are cross compiling... configure: error: in `/src/deps/libtool':
configure: error: cannot run C compiled programs.
If you meant to cross compile, use `--host'.
See `config.log' for more details
make: *** [Makefile:40: deps/libtool/Makefile] Error 1
emmake: error: 'make -j8' failed (returned 2)

The logs are expired but it clearly still worked in March when I made last update: https://github.com/GoogleChromeLabs/web-gphoto2/actions/runs/4338585581/job/11777833871

So weird...

RReverser commented 1 year ago

Ah. Here are extended logs from config.log:

configure:4044: /emsdk/upstream/emscripten/emcc -o conftest -Os -flto  -L/sysroot/lib -s DYNAMIC_EXECUTION=0 -s AUTO_JS_LIBRARIES=0 -s AUTO_NATIVE_LIBRARIES=0 -s ENVIRONMENT=web,worker -Os -flto conftest.c  >&5
configure:4048: $? = 0
configure:4055: ./conftest
internal/process/esm_loader.js:74
    internalBinding('errors').triggerUncaughtException(
                              ^

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension "" for /src/deps/libtool/conftest
    at new NodeError (internal/errors.js:322:7)
    at Loader.defaultGetFormat [as _getFormat] (internal/modules/esm/get_format.js:71:15)
    at Loader.getFormat (internal/modules/esm/loader.js:105:42)
    at Loader.getModuleJob (internal/modules/esm/loader.js:243:31)
    at async Loader.import (internal/modules/esm/loader.js:177:17)
    at async Object.loadESM (internal/process/esm_loader.js:68:5) {
  code: 'ERR_UNKNOWN_FILE_EXTENSION'
}
configure:4059: $? = 1
configure:4066: error: in `/src/deps/libtool':
configure:4068: error: cannot run C compiled programs.
If you meant to cross compile, use `--host'.
See `config.log' for more details

I think setting "type": "module" in package.json in this PR broke it as it now affects all files produced inside the project, even those generated by autotools.

RReverser commented 1 year ago

Ok fixed it by switching to "exports" field in package.json. It's somewhat newer, but I hope all modern bundlers will pick it up correctly.

RReverser commented 1 year ago

Heh I reverted version to 0.1.0 since I'm pretty sure this will require some more changes before considered 1.0, but then getting

npm ERR! 400 Bad Request - PUT https://registry.npmjs.org/web-gphoto2 - Cannot publish over previously published version "0.1.0".

I guess even if you delete package, the version restriction remains.

Oh well, I'll update to 0.2.0 or something.

RReverser commented 1 year ago

Ah... you published 0.2.0 before as well. Sigh :( I'll keep trying.

FWIW for the future if you're just testing you can use dependency via file:../path/to/package instead of publishing to npm. Or publish under a throwaway name maybe.

RReverser commented 1 year ago

Reached 0.4.0, looks like that finally worked.

icheered commented 1 year ago

I guess even if you delete package, the version restriction remains.

I did not expect that... I did start using a throwaway name but as this is the first time I worked on publishing through NPM I've been learning a lot about the process

Reached 0.4.0, looks like that finally worked.

Awesome! Luckily there are infinite sub-versions available before publishing a major release ;)

RReverser commented 1 year ago

Heh yeah. I just published 0.4.1 too as apparently needed few more fields for it to resolve on unpkg. Seems to work now, fingers crossed for the demo deploy.

icheered commented 1 year ago

I'll test it later tonight and will try to have a svelte demo PR ready soon. Thanks for you help on the project, I'm happy I could contribute :)

RReverser commented 1 year ago

Whew the demo seems to work.