Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

feat(gulp): add wiredep gulp task #1223

Merged
merged 1 commit into from
Mar 11, 2016
Merged

feat(gulp): add wiredep gulp task #1223

merged 1 commit into from
Mar 11, 2016

Conversation

itelo
Copy link
Contributor

@itelo itelo commented Feb 18, 2016

No description provided.

@mleanos
Copy link
Member

mleanos commented Feb 18, 2016

@itelo Can you give a brief explanation of what this task does? Also, there's no Grunt task with this functionality right?

@itelo
Copy link
Contributor Author

itelo commented Feb 18, 2016

when you download some bower components you just run:

gulp wiredep

then, all needed css and js paths will to the config/assets/default.js.

ignorePath: '../../',
fileTypes: {
html: {
replace: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what this is doing. Can you elaborate on your explanation? I'm curious why the script & link directives come into play here.

@mleanos
Copy link
Member

mleanos commented Feb 18, 2016

I like the idea of this task. However, I'm still a little unclear on the actual implementation of the wiredep task here, with the html replace option.

@itelo
Copy link
Contributor Author

itelo commented Feb 18, 2016

@mleanos sorry, don't need this html there...

@ilanbiala
Copy link
Member

Can you show what the file would look like after running gulp wiredep?

@itelo
Copy link
Contributor Author

itelo commented Feb 18, 2016

'use strict';

module.exports = {
  client: {
    lib: {
      css: [
        // bower:css
        // endbower
      ],
      js: [
        // bower:js
        'public/lib/jquery/dist/jquery.js',
        'public/lib/es5-shim/es5-shim.js',
        'public/lib/angular/angular.js',
        'public/lib/angular-animate/angular-animate.js',
        'public/lib/angular-bootstrap/ui-bootstrap-tpls.js',
        'public/lib/angular-file-upload/angular-file-upload.js',
        'public/lib/angular-messages/angular-messages.js',
        'public/lib/angular-resource/angular-resource.js',
        'public/lib/angular-ui-router/release/angular-ui-router.js',
        'public/lib/angular-ui-utils/ui-utils.js',
        'public/lib/bootstrap/dist/js/bootstrap.js',
        'public/lib/owasp-password-strength-test/owasp-password-strength-test.js',
        // endbower
      ],
      tests: ['public/lib/angular-mocks/angular-mocks.js']
    },
    css: [
      'modules/*/client/css/*.css'
    ],
...

there have a problem with css in bootstrap bower.json, should I fix that?

@itelo
Copy link
Contributor Author

itelo commented Feb 18, 2016

after run:

bower install --save angular-material

then:

gulp wiredep

default.js looks like this:

'use strict';

module.exports = {
  client: {
    lib: {
      css: [
        // bower:css
        'public/lib/angular-material/angular-material.css',
        // endbower
      ],
      js: [
        // bower:js
        'public/lib/jquery/dist/jquery.js',
        'public/lib/es5-shim/es5-shim.js',
        'public/lib/angular/angular.js',
        'public/lib/angular-animate/angular-animate.js',
        'public/lib/angular-bootstrap/ui-bootstrap-tpls.js',
        'public/lib/angular-file-upload/angular-file-upload.js',
        'public/lib/angular-messages/angular-messages.js',
        'public/lib/angular-resource/angular-resource.js',
        'public/lib/angular-ui-router/release/angular-ui-router.js',
        'public/lib/angular-ui-utils/ui-utils.js',
        'public/lib/bootstrap/dist/js/bootstrap.js',
        'public/lib/owasp-password-strength-test/owasp-password-strength-test.js',
        'public/lib/angular-aria/angular-aria.js',
        'public/lib/angular-material/angular-material.js',
        // endbower
      ],
      tests: ['public/lib/angular-mocks/angular-mocks.js']
    },
    css: [
      'modules/*/client/css/*.css'
    ],
...

it's important to notice that wiredep only work if you save the package in the bower.json

@mleanos
Copy link
Member

mleanos commented Feb 18, 2016

@itelo Thank you. I'm definitely a fan of this addition. When I first started using the framework, it wasn't very clear to me that I needed to add the dependency to the asset file.

Should this be added to the production asset file as well?

@itelo
Copy link
Contributor Author

itelo commented Feb 18, 2016

@mleanos thanks, do you think that 'gulp wiredep' should work to prod and default? or a each command to them?

@mleanos
Copy link
Member

mleanos commented Feb 18, 2016

That's a good question. I can see benefits to doing it either way.

@ilanbiala WDYT?

One one hand, it would be nice for the process to be as automated as possible. Meaning that adding them to both the prod & default assets would be very convenient. However, it may make sense that a user would want to have more control over it; adding it to just default at first for testing.

If we kept them as separate tasks, something like:
gulp:wiredep (default) & gulp:wiredep:prod then we could include the latter as part of the build task.

@mleanos
Copy link
Member

mleanos commented Feb 18, 2016

Side note: What happens when a dependency is removed from bower.json? Does the task just reflect what is in the Bower config? Or does it just look at the difference & adds if there is something new?

@itelo
Copy link
Contributor Author

itelo commented Feb 18, 2016

wiredep look to bower.json and everything the package need, if you remove and run wiredep again they will update default.js

@itelo
Copy link
Contributor Author

itelo commented Feb 18, 2016

if we really automatic the process, what do you think to add wiredep in bowerrc too? so when someone download a package, they will automatic to default and/or prod (:

@mleanos
Copy link
Member

mleanos commented Feb 18, 2016

@itelo It's not a bad idea to add the wiredep task to the bowerrc config, but I don't see it really being that beneficial. If we knew that our user's would be installing/uninstalling Bower components using the bower commands, then it might make more sense.

My hunch is that most user's will use bower install but not bower uninstall. If we include the Gulp task in the Bower configuration, I don't think it could be truly automated; only one way :)

I think requiring our User's to be actively involved in managing their front-end dependencies, would be best. Otherwise, we might be over complicating things for a small edge case of user's.

I'm not opposed to it though, if others think we should do it.

@trainerbill
Copy link
Contributor

Um its is very beneficial. Great job @itelo. I did the same thing in my fork but I use bower main files to get them and concatinate to vendor.js. This is how I would go about it, but your implementation is the only way it would be approved here. nice work

@mleanos
Copy link
Member

mleanos commented Feb 18, 2016

@trainerbill What are you saying is very beneficial?

This PR in general? Or adding it to the .bowerrc config. If the latter, could you touch up on my points?

@ilanbiala
Copy link
Member

@trainerbill why would bower main files not work?

@itelo
Copy link
Contributor Author

itelo commented Feb 18, 2016

@trainerbill, "I use bower main files to get them and concatinate to vendor.js", I think wiredep already do that... or I am wrong ?

@trainerbill
Copy link
Contributor

@ilanbiala I use a plugin called main-bower-files. Its lighterweight and does not inject like wiredep. I use gulp-inject for that.

@itelo Yes it will work and wiredep is probably more in line with how this project is currently setup. wiredep is an injector and read bower files plugin. I prefer gulp-inject so i use that in combination with main-bower-files. So in my application, i concatinate all the vendor files from main-bower-files, minify, and use gulp inject at run time. It makes the application more efficient then including all the files separately like your implementation does. Also the scripts are directly injected into the layout in core/server instead of the dumb template loop that includes the scripts individually. But like I said, with the way MEAN is currenly structured wiredep is probably best as it does not require a massive change.

@mleanos
Copy link
Member

mleanos commented Feb 18, 2016

@ilanbiala WDYT about @itelo's idea of adding this to the .bowerrc config, and whether or not we should have two seperate scripts for the default & prod asset files?

Please see my comments above.

@ilanbiala
Copy link
Member

Definitely two separate scripts, on should bring in uncompressed for dev, and another compressed for production. What does adding it to .bowerrc do exactly?

@itelo itelo force-pushed the gulp branch 2 times, most recently from 8a8f384 to fe79aac Compare February 19, 2016 04:55
@itelo
Copy link
Contributor Author

itelo commented Feb 19, 2016

I added the wiredep:prod task (:

@itelo
Copy link
Contributor Author

itelo commented Feb 19, 2016

@ilanbiala wiredep in bowerrc will just add the filePath to default and prod assets when you run:

bower install --save somepackage

@mleanos
Copy link
Member

mleanos commented Feb 24, 2016

@itelo I think this looks good. Anyway you can rebase? I'd like to pull this down & test.

@trendzetter
Copy link
Contributor

Sorry it's not bad but it feels more like gadgetry to me. I am not sure if this simplifies the development for beginners. Yet another trick you have to know, magic happening. It's highly recommended that you know what happens behind the scene so it's not that it is masking complexity.
And for more seasoned developers, how many dependencies do you need to add to bower?

@mleanos
Copy link
Member

mleanos commented Feb 28, 2016

@ilanbiala I thought it has added extra files as well, then I realized it was just adding files for packages that were already in the Bower.json, but not yet in the Assets file. If not that's not the case for yourself, can you give an example of what files you don't think should have been added? It also, re-sorts the files to be alphabetical.

@ilanbiala
Copy link
Member

es5 shim was one, I'm not sure if we use ui-utils.

@mleanos
Copy link
Member

mleanos commented Feb 29, 2016

That is interesting. Although, if you look in the public/lib folder you'll see that both es5 shim & jquery are installed there, on master. I can't remember why, but those dependencies get installed as stand-alone but I think they are required by other dependencies.

@itelo Can you shed some light on that?

@trendzetter
Copy link
Contributor

I must have had a bad day the last time I posted a bit uninterested comment on this. On second thought I think it's really cool if you would add a few lines to the docs so this does not end up as some hidden insiders trick.

@mleanos
Copy link
Member

mleanos commented Mar 5, 2016

@itelo Can you look into the possible issues that Ilan & I mentioned, about extra files that might be added?

@itelo
Copy link
Contributor Author

itelo commented Mar 5, 2016

I checked and i'm have one question, MEAN use jquery.js and es5-shim.js ?

@mleanos
Copy link
Member

mleanos commented Mar 5, 2016

I believe es5-shim won't install anymore because we removed ui-utils. As for JQuery, I believe it's a dependency of Bootstrap and/or Angular. However, it installs as a stand-alone. I'm not 100% sure on that, and don't know why. After installing the Bower dependencies, you'll notice that jquery got installed. I guess wiredep somehow knows this, or looks at the install directory. But I'm pretty sure jquery should be in the assets configurations.

}
},
"resolutions": {
"angular": "~1.3"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We recently removed this configuration when upgrading to Angular 1.5. I'm not sure why it was removed, but I was having issues locally installing Bower with the package's postinstall script, because the version of Angular could not be resolved. My vote is to have this added back, but I think we should set it to "~1.5".

@codydaig @ilanbiala @lirantal

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mleanos does Bower print out anything? It should usually say why it resolved to some version.

@itelo
Copy link
Contributor Author

itelo commented Mar 5, 2016

i re-checked, and es5 go away after rebase, and I agree if @mleanos about Jquery.

@simison
Copy link
Member

simison commented Mar 5, 2016

@mleanos Bootstrap uses jQuery for its JS bits. That's the bad side about automated wiredep — you might end up loading libraries you don't actually use (if you use only CSS parts of Bootstrap).

Angular uses jQuery if it is present:

Does Angular use the jQuery library?
Yes, Angular can use jQuery if it's present in your app when the application is being bootstrapped. If jQuery is not present in your script path, Angular falls back to its own implementation of the subset of jQuery that we call jQLite.
https://docs.angularjs.org/misc/faq

@mleanos
Copy link
Member

mleanos commented Mar 5, 2016

@simison Thank you for clarifying that.

By the sound of it, Angular will only use JQuery if we add it to our assets config. Is that correct?

Otherwise, if Angular will use it whenever it's installed (regardless of it being in the assets config), then that could cause a difference in implementation between a development environment & a production environment; I doubt that's the case though. That's just speculation.

@itelo Would it be possible to exclude JQuery from wiredep's automation?

@simison
Copy link
Member

simison commented Mar 5, 2016

By the sound of it, Angular will only use JQuery if we add it to our assets config. Is that correct?

Correct.

Actually on second thought, regular Bootstrap uses jQuery but Angular UI-Bootstrap doesn't; they've written directives fully Angular way.

So yea, ditch that jQuery! :-)

@itelo
Copy link
Contributor Author

itelo commented Mar 6, 2016

removed jquery

}
},
"resolutions": {
"angular": "~1.3"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilanbiala What should we do about this Angular version resolution? I know we recently removed it from bower.json, when we upgraded to Angular 1.5. However, it seems that we still have this issue. On master I'm prompted to select a version of Angular to resolve to when I install the Bower dependencies.

Do we want to keep this here? If so, I think it should be set to ~1.5. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mleanos it should say what dependency cannot resolve the correct version of Angular. Let's fix that since that is the root cause of the issue rather than just resolving it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I'm not able to reproduce the issue on my side. Even after doing bower cache clean. I guess we can remove the resolutions config here, and roll with it.

@itelo Is there any particular reason/need for it to be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, should I remove the resolutions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please :)

@mleanos
Copy link
Member

mleanos commented Mar 6, 2016

LGTM. @simison?

I'd like to wait to see what @ilanbiala has to say about the Angular version resolutions.

@mleanos
Copy link
Member

mleanos commented Mar 7, 2016

LGTM. I'll merge in a day or two, if nobody has any objections. Thank you @itelo.

Side Note: @ilanbiala We're ignoring the bitHound failures correct?

@mleanos mleanos added this to the 0.5.0 milestone Mar 7, 2016
@ilanbiala
Copy link
Member

Yes, you can ignore BitHound and LGTM.

@mleanos
Copy link
Member

mleanos commented Mar 10, 2016

@itelo Can you rebase, since there are conflicts now? I'll merge as soon as you do. Thanks.

@mleanos
Copy link
Member

mleanos commented Mar 11, 2016

Merging. Thank you @itelo

mleanos added a commit that referenced this pull request Mar 11, 2016
feat(gulp): add wiredep gulp task
@mleanos mleanos merged commit 9d4249d into meanjs:master Mar 11, 2016
@simison
Copy link
Member

simison commented Mar 12, 2016

Whoop, Wiredep just got deprecated notice added to its readme: https://github.com/taptapship/wiredep

@ilanbiala
Copy link
Member

@mleanos @itelo what should we do? We shouldn't really be using deprecated packages.

@mleanos
Copy link
Member

mleanos commented Mar 12, 2016

We definitely don't want to rely on a package that isn't maintained any longer. However, I think we can sit back for a short time to see if someone steps up & takes the reigns on that project. In the meantime, we could be looking for an alternative solution. I think it might be too soon to worry about it a whole lot. After all, the wiredep project has 800+ stars so there might be willing contributors to take it over.

Thanks for pointing this out @simison What would your suggestion be?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants