-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
modularized #66
base: master
Are you sure you want to change the base?
modularized #66
Conversation
Wow, this is a huge change! You've basically rewritten the entire library, LOL. Either that, or GitHub just can't make heads or tails of the diff. It looks like virtually every line is changed. Since I don't use any module loaders, this is a difficult pill for me to swallow. I don't want to have to use npm and browserify to install my library. I just want to drop the files into place and have it work. I guess I'll have to think about this. Perhaps for v2.0? Seriously tho, thank you for all the time you put into this. This is a huge amount of work for a free, open source library. I have half a mind to just merge the PR, but I'm really afraid this is going to break everyone's implementations, and I'll have to spend hours learning about how these module loaders work, rewrite the docs, fix all the test pages, etc. etc. And then if people have problems installing or using it I won't be able to help. It's just going to be a huge time investment that I just can't make right now. But I will absolutely make it my goal to learn about these module loaders for v2.0, and then merge this PR, or code up something similar. Thanks again! I'm leaving this thread open.
|
This change doesn't require you to use a module loader. It's totally optional. I wrote the test with modules because I wanted to be sure it worked. The module loader bit didn't change at all, actually. I just moved some things around to allow instance creation, and modularity by proxy. Any problems surrounding the module compatibility code will have existed before this change (it's pretty simple, and I don't think it's cause for concern). I moved all of the functions into The main breaking change is how the webcam is set up. Instead of a singleton (singleton methods no longer exist, btw), you will create instances like this:
Each instance needs an external interface method for Thanks for considering the merge. If it helps, I can watch this repo and respond to github issues as they arise. |
Okay, thanks for the explanation, and sorry for my misunderstanding. I'll do some more research, so I can properly understand these changes. I have to say tho, this feels like a v2.0 rewrite to me, so maybe I'll just start over from scratch, and take into account multiple cameras and module loaders from the get go. Thanks again for all your help! |
Yeah, it's definitely something for a major release. For now, though, I recommend removing the module-compatibility block because it breaks the flashNotify interface. |
I'm hoping this commit will help with that: |
Hi this is #47 again I was able to follow your guide
The Trouble I got is this: The first two createWebcam with 'force_flash: true' option (line 29 ~ 39 of your index.js file, displayed below) capture the same images, ie images are taken by the same webcam, even I selected different webcams. createWebcam({ createWebcam({ Would you please give me an advice? Thanks a lot. |
I hadn't tried multiple webcams, but it should be possible. You will need to update the https://github.com/jhuckaby/webcamjs/blob/master/webcam.js#L367-L373 |
@myongjailee how did you manage to use 2 webcams? |
Resolves #63. Actually resolves #45. Related to #47.
Requires browserify to build. Install with
npm install -g browserify
, then runtests/modularize/build.sh
.The test uses commonjs modules, but it should behave the same across module systems if the wrapper works correctly (it seems to). The test creates four instances of the webcam; two of rtc and two of flash. No problems, so far.
It's a breaking change, so might be something for the next version. Docs and stuff will need updates.
Give it a spin and let me know what you think!