yeoman install backbone deletes existing files in app/scripts/vendor #460

Closed
latentflip opened this Issue Sep 15, 2012 · 36 comments

Comments

Projects
None yet
Contributor

latentflip commented Sep 15, 2012

I think this might be related to PR 408 but I can't be sure.

  • First I create a new app yeoman init, answering y,y,y,n,n.
  • Do a git init and commit
  • Then I install backbone yeoman install backbone.
  • And for some reason doing so has deleted a bunch of stuff in app/scripts/vendor:

Here's my git status after running yeoman install backbone

# On branch master
# Changes not staged for commit:
#   (use "git add/rm <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#   modified:   app/scripts/main.js
#   deleted:    app/scripts/vendor/bootstrap/bootstrap-affix.js
#   deleted:    app/scripts/vendor/bootstrap/bootstrap-alert.js
#   deleted:    app/scripts/vendor/bootstrap/bootstrap-button.js
#   deleted:    app/scripts/vendor/bootstrap/bootstrap-carousel.js
#   deleted:    app/scripts/vendor/bootstrap/bootstrap-collapse.js
#   deleted:    app/scripts/vendor/bootstrap/bootstrap-dropdown.js
#   deleted:    app/scripts/vendor/bootstrap/bootstrap-modal.js
#   deleted:    app/scripts/vendor/bootstrap/bootstrap-popover.js
#   deleted:    app/scripts/vendor/bootstrap/bootstrap-scrollspy.js
#   deleted:    app/scripts/vendor/bootstrap/bootstrap-tab.js
#   deleted:    app/scripts/vendor/bootstrap/bootstrap-tooltip.js
#   deleted:    app/scripts/vendor/bootstrap/bootstrap-transition.js
#   deleted:    app/scripts/vendor/bootstrap/bootstrap-typeahead.js
#   deleted:    app/scripts/vendor/jquery.min.js
#   deleted:    app/scripts/vendor/modernizr.min.js
#   deleted:    app/scripts/vendor/require.js
#
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#   .sass-cache/
#   app/scripts/vendor/backbone/
#   app/scripts/vendor/underscore/
#   components/

From looking through the code, it seems that the bower install task is running the bower:sync helper, which removes the contents of app/scripts/vendor and copies everything over from components/, which would work in theory, but it looks like when the first yeoman init is done, it's not putting the files into components/ (it doesn't use the bower tasks perhaps)?

Contributor

latentflip commented Sep 15, 2012

Hmm, this may also be related to #323, but I don't think it's exactly the same problem.

Owner

addyosmani commented Sep 16, 2012

Ahh. I see what the issue is. The changes we made to enable Bower components to synchronize up with the vendor directory didn't take into account other generators that may also be placed scripts into that location, but not via Bower itself.

@mklabs I'm thinking the solution to this might be opting for a different directory where we place the Bower scripts (e.g app/scripts/components or app/scripts/vendor components). We want somewhere that ideally will be reserved for your Bower packages. Does that make sense? If so, lets try identifying what (beyond the sync path) needs to be changed to facilitate this. Maybe the custom bower path in the gruntfile too?

Contributor

latentflip commented Sep 16, 2012

Splitting out the Bower deps could help with #463 too.
On Sep 16, 2012 11:10 PM, "Addy Osmani" notifications@github.com wrote:

Ahh. I see what the issue is. The changes we made to enable Bower
components to synchronize up with the vendor directory didn't take into
account other generators that may also be placed scripts into that
location, but not via Bower itself.

@mklabs https://github.com/mklabs I'm thinking the solution to this
might be opting for a different directory where we place the Bower scripts
(e.g app/scripts/components or app/scripts/vendor components). We want
somewhere that ideally will be reserved for your Bower packages. Does that
make sense? If so, lets try identifying what (beyond the sync path) needs
to be changed to facilitate this. Maybe the custom bower path in the
gruntfile too?


Reply to this email directly or view it on GitHubhttps://github.com/yeoman/yeoman/issues/460#issuecomment-8600247.

Contributor

mklabs commented Sep 16, 2012

Using a separate directory for bower deps might help indeed. I would vote for app/scripts/vendor/components. The custom bower path in the gruntfile might be removed, if we handle the defaults. The documentation for the bower task can then indicate that it can be configured through bower.dir config value in the gruntfile.

+1 for vendor/components.. makes it simple to reference from requirejs paths key 'vendor/components/lodash/lodash.min' etc.

Owner

addyosmani commented Sep 17, 2012

I've spent some time reviewing our Gruntfiles and the impact that a change over to app/scripts/vendor/components might have. There are some complexities (from what I can see) in us doing this:

  • Across watch, lint, compass etc. I can't figure out an easy way for us to include all of the scripts within vendor with the exception of the components directory. I also question how clean it is (if this is possible) to include an exclude for it within each task block in the Gruntfile. The exclude is necessary to avoid watching entire repos copied in via installation and the way Bower pulls dependencies. @mklabs @sindresorhus are you aware of how we could define directory excludes for a task? i.e i guess the opposite of files.
  • In practice, including the components directory within app/scripts/vendor/components felt a little weird to me for a few reasons: It's a long path name and will be even longer once you factor in the component name and library. I personally wouldn't want to type app/scripts/vendor/components/packageName/library.js. It also felt fragile sharing a sub-directory where we have both non-Bower assets and Bower assets being included.
  • Including a components directory within scripts assumes that the content being pulled in via bower will always be a script. This may well not be the case as the component format could easily just be styles, html or anything else.

With the PR I've submitted above to the generators repo, I've changed my recommendation to us opting for app/components.

I feel this provides a cleaner separation between what is expected of the scripts directory and Bower components. The watch task and so on would no longer have to keep an eye on all of the repo files pulled down by dependencies, however users could still reference scripts in app/components and have them correctly included in the final dist version of their app (i.e we wouldn't have the current issue of watch and build trying to process everything pulled down in those component repos).

Not sharing the /scripts directory also has the impact of us not having to worry about dependency clobbering. We document that components is fully owned by Bower and should ideally be left to it for manage.

This would solve #463 as well.

(Note to self: If this is accepted, we need to update the RequireJS config wiring to reference app/components instead).

cc @paulirish

Member

paulirish commented Sep 17, 2012

I've changed my recommendation to us opting for app/components.

This is truer to the intended design of Bower. And reinforces that components need not be script-based (though usually are). As a design decision this keeps maintenance a bit more straightforward.. I dont think users will neccessarily like this change and most will probably dislike it. But I'm +1 on it regardless.

Because of the auto-updating nature update of Yeoman it might be tricky to land this without having https://github.com/mklabs/yeoman/tree/configpaths in place. Aye?

Owner

addyosmani commented Sep 17, 2012

@paulirish Happy you're +1 on this :) I think it would make sense for us to land this at the same time as the config work.

We could consider branching the generators repo with the PR to change the paths in place for anyone wanting a more immediate fix, otherwise I'd be on board with this going into 0.9.2.

Owner

sindresorhus commented Sep 17, 2012

I'm happy with this change too. Just make sure tests are updated and that it compiles and moves script/css etc nicely.

Contributor

latentflip commented Sep 17, 2012

If this fixes this issue, as well as the issue about all of app/vendor getting built that sounds great.

A thought though: won't this still result in the dist/ directory ending up with the full contents of the app/components/ directory? If it's just being copied over, and not getting minified etc I guess it's not so bad, and maybe it can be prevented in .buildignore?

As an alternative: I may be missing something, but isn't the point of the main property in a bower package's package.json to specify which files in the package are actually the ones being released, rather than supplementary docs, dev files, etc? Could we use that file list to only copy the needed files over from ./components to ./app/components to prevent all that extra cruft ending up in a users app, or is there a reason that won't work?

Owner

addyosmani commented Sep 17, 2012

@sindresorhus roger on that.

@latentflip We all originally thought that the main property would solve a lot of these issues and avoid having to handle the entire project repo for dependencies that you pull down. Unfortunately it's been confirmed recently that this isn't the case and Bower copies everything because you may have other scripts/content being pulled in via your main scripts.

My proposal doesn't necessarily have to mean copying over everything from app/components - we could set it up so that only those scripts/content which you reference in your app get included in the build. I'm personally opposed to copying over cruft unless its totally necessary.

addyosmani was assigned Sep 17, 2012

Contributor

latentflip commented Sep 17, 2012

@addyosmani sounds good. I guess if people are using require.js, or have the files they want explicitly referenced in their gruntfile as part of the minification/concatenation section, we can just ignore app/components completely as part of a build as they will get pulled in anyway.

Apologies for keeping bringing up ideas you guys have gone through before, got some catching up to do :)

Philip Roberts
phil@latentflip.com
@philip_roberts

On Monday, 17 September 2012 at 11:30, Addy Osmani wrote:

@sindresorhus (https://github.com/sindresorhus) roger on that.
@latentflip (https://github.com/latentflip) We all originally thought that the main property would solve a lot of these issues and avoid having to handle the entire project repo for dependencies that you pull down. Unfortunately it's been confirmed recently that this isn't the case and Bower copies everything because you may have other scripts/content being pulled in via your main scripts.
My proposal doesn't necessarily have to mean copying over everything from app/components - we could set it up so that only those scripts/content which you reference in your app get included in the build. I'm personally opposed to copying over cruft unless its totally necessary.


Reply to this email directly or view it on GitHub (#460 (comment)).

krulik commented Sep 19, 2012

Just wanted to note I had the same problem when installing the html5-boileplate package to /components. It removed bootstrap plugins and modernizr from /vendor.

Just reporting I have the same issue on yeoman v0.9.1

This is What I do:
yeoman init
only select bootstrap and bootsrap plugins
yeoman install angular

and scripts/vendor folder only has angularJS in it. Others are deleted. This started to happen on 0.9.1 for me

same issue after yeoman install backbone-amd
v 0.9.1

mshick commented Sep 21, 2012

Also a problem with the ember generator and subsequent "yeoman install ember-data"

Having the same issue ended up creating opening another issue at - yeoman/yeoman.io#23

If I run yeoman init angular
Everything is fine I get the angular seed project setup.

But then if I want to add bootstrap and run yeoman install bootstrap

It removes all the files in the vendor folder including the angular.js files and just adds the bootstrap folder. And it does not update the index.html file not sure if it should.

I understood the install method lets you add packages to your project.

It seems you have to do the following init angular then install all your packages jquery, bootstrap, ect. and then go back and yeoman init angular --force once that is done add the script tags to your html manually.

AlmogRnD referenced this issue in yeoman/yeoman.github.io Sep 22, 2012

Closed

Overwriting existing files when using install method #23

Also note I'm packages installed both in app/components and in app/vendor not sure why.

If I run yeoman init angular:all and run yeoman install package for example angular-ui I get the following

angular vendor files are removed and angular-ui is installed inside vendor but also added to app/components this is very confusing seeing it in two different places.

mshick commented Sep 22, 2012

I didn't go into as much detail as I might have, since I was really just +1ing this convo, but like @almogdesign I am also seeing vendor files show up in both places, and having the usable, "compiled" files removed from scripts/vendor.

I think it would make sense for the install routine to add vendor files to components, then place a dev ready version (IE, ember-1.0.min.js) into the scripts/vendor folder.

I think seeing the same files in vendor and in components is a bit confusing, I suggest one place. The important thing is being able to run yeoman init backbone, angular ect... and then later on add packages without any issues.

For me this very important I see myself adding packages like bootstrap or angular UI as I move forward within the project. I don't like the idea of having to use --force it rewrites my index.html

Owner

addyosmani commented Sep 22, 2012

Thanks for the reports, folks. Just to confirm: this is currently an issue that will affect all generators/configs but we are aiming to address it for the next release.

@addyosmani is there a time table for the next update / fix wondering if I should wait a bit worried how this will affect an existing project.

Owner

addyosmani commented Sep 22, 2012

We will probably document a manual fix this week to ensure this minimises the effects on existing projects. I would estimate next release in a week or two otherwise.

Sent from my iPad

On 22 Sep 2012, at 19:14, Almog Koren notifications@github.com wrote:

@addyosmani is there a time table for the next update / fix wondering if I should wait a bit worried how this will affect an existing project.


Reply to this email directly or view it on GitHub.

A manually fix would be great thanks for the great support makes all the difference when choosing a framework.

Rydgel commented Sep 22, 2012

+1 for the manually fix :)

addyosmani referenced this issue in yeoman/generator Sep 24, 2012

Closed

Generator fix for yeoman/issues/460 #65

Owner

addyosmani commented Sep 24, 2012

fyi: build appears to be failing though not entirely sure why:

Done, without errors.
625  ✓ should install jquery with yeoman install jquery (4110ms)
626    ◦ should have copied resolved components to app/components:   ✓ should have copied resolved components to app/components 
627    ◦ should install backbone with yeoman install backbone: 
628Running "bower:install:backbone" (bower) task
629GET https://bower.herokuapp.com/packages/backbone
630bower cloning git://github.com/documentcloud/backbone.git
631bower caching git://github.com/documentcloud/backbone.git
632  1) should install backbone with yeoman install backbone
633    ◦ should have copied resolved components to app/components: <FATAL> Cannot read property 'source' of undefined� </FATAL>
634npm ERR! Test failed.  See above for more details.
635npm ERR! not ok code 0
636
637Done. Build script exited with: 1

jQuery copies/installs fine but Backbone doesn't. I don't have this issue locally. Will chase, though if anyone can reproduce the above that would be helpful.

@addyosmani I just cloned the repo and get the same error:

$ npm test

> yeoman@0.9.2 test /Users/dignifiedquire/opensource/yeoman/cli
> node node_modules/mocha/bin/mocha test/test-*.js



  Bower install packages
    ◦ should install jquery with yeoman install jquery: 
Running "bower:install:jquery" (bower) task
GET https://bower.herokuapp.com/packages/jquery
bower cloning git://github.com/components/jquery.git
bower caching git://github.com/components/jquery.git
bower fetching jquery
bower checking out jquery#1.8.1
bower copying /Users/dignifiedquire/.bower/jquery
bower Updating RequireJS config: app/scripts/main.js 

Done, without errors.
    ✓ should install jquery with yeoman install jquery (10215ms)
    ✓ should have copied resolved components to app/components 
    ◦ should install backbone with yeoman install backbone: 
Running "bower:install:backbone" (bower) task
GET https://bower.herokuapp.com/packages/backbone
bower cloning git://github.com/documentcloud/backbone.git
bower caching git://github.com/documentcloud/backbone.git
    1) should install backbone with yeoman install backbone
    ◦ should have copied resolved components to app/components: <FATAL> Cannot read property 'source' of undefined </FATAL>
npm ERR! Test failed.  See above for more details.
npm ERR! not ok code 0

Is this fixed, wanted to know if I can start installing packages?

Hi, I'm hoping for a fix as well. For now, I've been manually adding the bootstrap / angular-ui libs as needed. Thanks for such an awesome tool! I started using it yesterday and I am already fairly confident and up-to-speed on developing.

sephie commented Oct 12, 2012

Not sure if this is directly related to this issue, but while watching The Breakpoint yesterday I noticed that Brian used "yeoman init angular" and that it ran differently than on my machine; the Y/N Bootstrap questions were included, while I only get those when using plain "yeoman init."

+1 it's just deleted all my vendor folder contents

+1

Owner

sindresorhus commented Jan 31, 2013

Fixed in 1.0 which will be out soon.

@szinya szinya pushed a commit to menthainternet/yeoman that referenced this issue Sep 17, 2014

@addyosmani addyosmani Updates Bower test to be aligned with #460 14ad9cd

@szinya szinya pushed a commit to menthainternet/yeoman that referenced this issue Sep 17, 2014

@addyosmani addyosmani Updating docs for #460, minor comment tweak for bower test 8c0f52b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment