search

jplayer / jPlayer

jPlayer : HTML5 Audio & Video for jQuery
http://jplayer.org/
Other
4.6k stars 1.47k forks source link

Do not use minified version for node/browserify package #258

Closed nervo closed 9 years ago

nervo commented 9 years ago

Browserify users don't want the minified version of a package. Minifying it is a part of their custom build process.

nervo commented 9 years ago

Hm, i didn't see that src folder is excluded in package.json (so is circle.player skin). Maybe you can just provide a minified and a non minified js version in js/jplayer folder

thepag commented 9 years ago

circle player should not even be in this repository. It is maintained in another repository, hence why its code is in the /lib/ folder. It would not be in the repo at all if it were not for it being one of the demos.

259 i also talking about the missing source... Well, missing from the place people want it... I do not like the idea of duplicating the file, but I do not might doing that, as long as people realise that it is a copy of the source and not the one to change and ifssue PRs to, which is going to happen all the time since that is the one they use. Bah. Doing other stuff today at 100mph so sorry for rushed answe.

slavafomin commented 9 years ago

@thepag I'm not seeing any practical problems with duplicate file, but if you like, you can create a symlink for it. I'm a DRY-terrorist myself ))

nervo commented 9 years ago

A better approach: as your are using grunt, you can let a raw js file in src, and use appropriate grunt contrib plugins to add dynamic header (getting things like version from package.json) and surround code with module loader. This way, you keep a clean src file, and can expose a not minified and minified version of your dist lib.

slavafomin commented 9 years ago

@nervo I agree. That would be even better. @thepag, with this approach, you could even split source code into separate files for ease of development and patch them together during build into a single distribution file.

thepag commented 9 years ago

You are right. Part of the reasoning behind the grunt build and the repo refactor were so that we could start to break up the source files and maybe give build options at some time in the future.

I'll be making changes today, renaming /js/ to /dist/ for the built output and I will add in the original JS too for each of the other bits too. I'll be changing our website at the same time, albeit under the hood changes to match the repo structure.

thepag commented 9 years ago

FYI, I will not be merging this PR.

nervo commented 9 years ago

np, as long as the source file is available in y node_modules (and the commonjs loader too :))

thepag commented 9 years ago

I believe the issues in this topic have been fixed. Currently only in the v2.8 branch, but will tag and merge with master later today.

slavafomin commented 9 years ago

Thanks @thepag!

slavafomin commented 9 years ago

P/S: @thepag have you considered generating a website/documentation content from the library repository automatically? It will take some time to implement, but it will allow to update the library much faster in the future.

thepag commented 9 years ago

The documentation in general could do with a refactor. While I strive to keep the doc up to date, and it is, a major refactor of how it is generated is not desirable right now. That would be a lot of effort and I have other commitments coming up. Maybe start a new issue @slavafomin covering the topic of documentation.

thepag commented 9 years ago

Having a copy of the source in the dist folder is breaking all of the pull requests and the code is merging with the dist folder stuff.

Brilliant!

Maybe I could fix it, but currently all pull requests are pretty useless.

slavafomin commented 9 years ago

Maybe the best course of action is to ask contributors to rebase their PRs against the current dev version.

thepag commented 9 years ago

I've been looking into git options and I cannot see a sensible way to distinguish between the new copy and the original. Git appears to look for filename and for diff and picks the closest, and setting the options higher will do little, since the files are the same.

I think the solution here is to rename the source file. Well, to rename either one, but the build dist folder is the correct filename convention for jQuery. Then during the merge, the 2 files will not look so identical. I'm going to test it out in a local branch first with that #150 PR I tried to merge earlier.

thepag commented 9 years ago

This appears to work for #150 so that the contributor does not need to do anything.

git checkout -b issue-150 master
git mv dist/jplayer/jquery.jplayer.js dist/jplayer/jquery.jplayer.src.js
git commit -am "rename dist src"
git pull https://github.com/sterlinghirsh/jPlayer.git master
sublime src/javascript/jplayer/jquery.jplayer.js
// Deal with conflict. ie., search for HEAD and edit it manually.
git commit -am "dealt with pauseOthers tellOthers refactor conflict."
git mv dist/jplayer/jquery.jplayer.src.js dist/jplayer/jquery.jplayer.js
git commit -am "rename dist src back again"
git diff master

The git diff master then only shows what it should, being the PR change with my fix due to pauseOthers being refactored into tellOthers in the meantime.

I'll see if I can drop the initial and final commit for the renaming of dist... But the above seemed to do the trick.

And that just leaves the following for me to merge the PR into master.

git checkout master
git merge --no-ff issue-150
git push
Afterster commented 9 years ago

Maybe dist should be considered as build files and removed from the repository? There is generally two ways to deal with it from what I can read, see the submodule way: http://blog.stackfull.com/2013/05/publishing-bower-components/

slavafomin commented 9 years ago

@Afterster that's a very convenient approach. @thepag maybe you can setup a jplayer-dist repository for distribution files and remove dist from this project? However, this is a breaking change, so major version should be updated.

nervo commented 9 years ago

-1 having a dist in the package is the way to go. Being in repo or not is just another question.

Afterster commented 9 years ago

Sure jPlayer should have a dist folder in the package, but I'm questioning the fact to have it in the repo (which is also related to the PR merge issue here). I don't have any experience with this, do you?

slavafomin commented 9 years ago

@nervo we were talking about removing "compiled" files from this repo. It will be still possible to build distribution files from this repo manually.

nervo commented 9 years ago

Sure. I made a distinction between the "package" (available on npm, for instance) and the github repo. The dist directory could clearly be generated localy, not available on github, and pushed on npm. But what about bower ? As far as i can remember, bower packages are just mapped from the git repo, no ?

Afterster commented 9 years ago

@nervo isn't submodule a valid solution for bower as describe in the posted link?

slavafomin commented 9 years ago

@nervo with this approach there will be another GitHub repository for distribution files. Consider example with AngularJS: https://github.com/angular/bower-angular.

nervo commented 9 years ago

Well.. why not :) Anyway, i'm not sure it's worth the pain..

thepag commented 9 years ago

Thank you for the discussion. I am going to have to leave the repo as is for the time being. Any new PRs should be fine, it is the older ones i need to fix during the merge process. I hope to get 2.9 out today, update the website and then that will be it for this push.