search

contently / videojs-annotation-comments

A plugin for video.js to add support for timeline moment/range comments and annotations
https://contently.github.io/videojs-annotation-comments/
Other
171 stars 50 forks source link

jQuery implicit dependency ? #44

Closed cdujeu closed 5 years ago

cdujeu commented 5 years ago

Hi guys, Very nice plugin here! I'd love to try to integrate it, but it seems to me that there is an implicit dependency to jQuery (and we don't want to load jQuery globally). Can you confirm me that, or am i wrong? Would it be possible to build a js file that would include $ (or maybe a more lightweight alternative) inside the build scope ? Thx!

jackpope commented 5 years ago

Thank you @cdujeu! Unfortunately, you are correct that there is an implicit jQuery global dependency here. I hope to some day refactor jQuery out of the plugin completely but it will require some work.

As for the scoping:

Quick(?) fix with webpack:

If you are using Webpack to load the module it should be possible to use the Imports Loader to inject your own local jQuery when importing. It would look something like this, although I haven't tried it myself.

const AnnotationComments = require('imports-loader?$=jquery,jQuery=jquery!videojs-annotation-comments');

Better fix:

I agree that the build should be smarter about this. When I have some time I can try to rework things to resemble this, where the library will default to require('jquery')and fall back to window.jQuery. jQuery would become an explicit peer dependency of this project until it can be refactored out.

If you have any ideas on how to best handle this, please let me know. I'll continue to look into solutions as I have time.

cdujeu commented 5 years ago

Hey Jack Thank you so much for your quick answer. I'm more of a grunt guy, reason why I was trying to use the build version directly ;-). So I'll need some time to digg into webpack (or gulp to rebuild the lib), but i'll definitely will try. I keep you posted. (edit: we are using grunt + browserify, and i had nasty errors trying to directly require the class, related to handlebars missing somewhere... But i did not spend that much time on it yet)

cdujeu commented 5 years ago

Jack, sorry again for bothering I'm trying to compile from source to do the jQuery injection, followed the step to npm install, then gulp, then when I run the compile tasks, the "transpile" tasks says no error but just does nothing (nothing in a newly created build folder). Any hints? thx!

jackpope commented 5 years ago

No worries. Are you running gulp as a global package? For some reason, when I try to run locally through yarn run or npx the compile and build commands don't work. It may be due to some incorrect relative paths in the gulp file... not sure at this point.

What does work for me is using the global package. If you follow these commands exactly do you still have trouble? https://github.com/contently/videojs-annotation-comments#develop-and-build

cdujeu commented 5 years ago

Yep that exactly what I did, installed gulp globally. The css are correctly compiled and appearing inside the build folder, but nothing for js. Is there a way to set a higher level of debug for gulp or a specific build?

cdujeu commented 5 years ago

Edit 1 : updating node/npm seems to build js files ! i'll go further and keep you posted, thanks Edit 2 : Adding $ = require('jquery') in the list of imports for each file that needs it does the trick. I also tweaked the index.js to just expose AnnotationComments to the global scope, and that does the job for me. I pushed on a forked version, not sure you want me to PR this, you can have a look at https://github.com/pydio/videojs-annotation-comments/commit/eb39957c1e647bcb54d60b89970bf7a773b348fd Closing for now, thanks for your great job!