marionettejs / backbone.marionette

The Backbone Framework
https://marionettejs.com
Other
7.07k stars 1.26k forks source link

bower dependencies is incorrect #1499

Closed MillerRen closed 10 years ago

MillerRen commented 10 years ago

I have a problem when i install marionette with bower. There is the dependencies in index.html /lib/backbone.marionette.js /lib/core/backbone.marionette.js I think it is duplicate because it is still worked when i remove the /lib/core/backbone.marionette.js manual.
Now the problem is that the files come back when i update the bower width bower update in yeoman.

samccone commented 10 years ago

Hi @MillerRen

lib/backbone.marionette.js

lib/core/backbone.marionette.js


Let me know if this is not clear

jamiebuilds commented 10 years ago

@samccone I think you missed this issue here, probably something to do with how yeoman handles bower deps.

samccone commented 10 years ago

Hmm have not used yeoman before, I am not sure

jamiebuilds commented 10 years ago

@MillerRen could you detail us on your yeoman setup?

samccone commented 10 years ago

ping @MillerRen

MillerRen commented 10 years ago

@thejameskyle I used generator-webapp. The bowerInstall task build in this generator. The index.html automaticly updated when the bower.json changed.

MillerRen commented 10 years ago

remove this line in main section of marionette's bower.json "./lib/backbone.marionette.js"

jamesplease commented 10 years ago

@MillerRen did ya mean to close this? :)

MillerRen commented 10 years ago

sorry. I'm a newer on github and my english is poor.

lsimoneau commented 10 years ago

Yeah, same problem here, in my case when using https://rails-assets.org/ I get both the core and non-core versions included in my bundled JS. The output of bower list --paths I believe should indicate only one of the two files, not both:

$ bower list --paths                                                                                                                                                    
{
  "backbone.babysitter": "bower_components/backbone.babysitter/lib/backbone.babysitter.js",
  "backbone.marionette": [
    "bower_components/backbone.marionette/lib/backbone.marionette.js",
    "bower_components/backbone.marionette/lib/core/backbone.marionette.js"
  ],
  "backbone.wreqr": "bower_components/backbone.wreqr/lib/backbone.wreqr.js",
  "backbone": "bower_components/backbone/backbone.js",
  "jquery": "bower_components/jquery/dist/jquery.js",
  "underscore": "bower_components/underscore/underscore.js"
}

Otherwise build tools like Yeoman or Rails-assets will think that both files need to be included.

lsimoneau commented 10 years ago

What is required to move forward on this issue? Would you accept a PR that limited the main section of bower.json to only the non-core version?

lsimoneau commented 10 years ago

Actually, now that I think about it, I think it should only include the core version, since both wreqr and babysitter are listed as dependencies already, so build tools will likely include them.

jamiebuilds commented 10 years ago

@lsimoneau Can you verify that build tools will include them?

lsimoneau commented 10 years ago

@thejameskyle the only thing I use is rails-assets, which does install generated gems for wreqr and babysitter when I install Marionette. I still need to include them separately in my app manifest. Bower definitely installs dependencies when you use bower install. I'm unfamiliar with Grunt and Yeoman so can't speak to those.

Bower's documentation is unclear about what these fields are supposed to represent, but I can only assume that dependencies should be interpreted literally, as in things the lib requires to run. It seems to me there are two possible approaches: specify the Marionette file that includes both Wreqr and Babysitter as the main component file for bower to install, or specify the core-only one as the main file, and the two other libs as dependencies. Given that it's possible that other libs might depend on e.g. Wreqr but not on Marionette, the latter seems much preferable, as it allows the package manager to do its job.

jamiebuilds commented 10 years ago

This is an inherent problem with bower, it lacks a proper definition of how main/dependencies fields should be handled by third parties for the browser. I think the only solution that will universally work is to use the bundled version for bower.json#main and remove babysitter and wreqr as dependencies.

samccone commented 10 years ago

deferring to @thejameskyle on this one

jamiebuilds commented 10 years ago

@samccone I wouldn't defer to me on this, I refuse to use bower in my applications because of problems like this. @jmeas uses it much more I believe so he would know better.

samccone commented 10 years ago

hahah ok

jamesplease commented 10 years ago

I manually set up the files after Bower pulls them in. I don't let anything resolve the dependencies for me.

@lsimoneau is it also pulling in Wreqr and Babysitter separately?

Basically we need to make our Bower file work. That might sound silly, but that means we'll also need to make something not work. Right now we're trying to support the bundled and unbundled versions. Let's just support...one of those.

So either the main file should be the bundle, and there are no Wreqr/Babysitter dependencies, or the main file will be the core, and there will be Wreqr/Babysitter under dependencies.

lsimoneau commented 10 years ago

@jmeas rails-assets installs the dependencies, yes. They still need to be added to the asset manifest separately, though.

I'm very much in favour of the bower config using the core file as main, and Wreqr/Babysitter under dependencies (which is what I did in my PR #1603).

If I write, e.g., an extension plugin for Wreqr, and list Wreqr as a dependency, and someone includes both it and Marionette in their project, this approach means the package manager and build tools can take care of business: Wreqr is a dependency of both projects, so they install it. If we went with the bundled version as main, then in that example the user would wind up with Wreqr installed as a dep of my lib, and also bundled in with Marionette.

jamesplease commented 10 years ago

:+1: Sound reasoning, @lsimoneau.

samccone commented 10 years ago

Fixed by https://github.com/marionettejs/backbone.marionette/commit/29d567b36369b7c43324bd9843efd2e0a071ee6a

MillerRen commented 10 years ago

Thanks!