Feaure(grunt): Wiredep task for grunt #1402

shanavas786 · 2016-07-16T13:13:06Z

Update dependencies and remove trailing comma from asset files.

Fixes #1360

remove explicit grunt task loading for `grunt-protactor-coverage` as `load-grunt-tasks` handles it

Automatically update bower dependencies.

Remove trailing comma from assets configuration generated by wiredep

Remove trailing comma from asset configurations generated by wiredep

ilanbiala · 2016-07-16T22:27:14Z

package.json

@@ -51,6 +51,9 @@
    "glob": "~7.0.0",
    "grunt": "~1.0.1",
    "grunt-cli": "~1.2.0",
+    "grunt-rtc": "^0.1.1",


shanavas786 · 2016-07-17T05:26:22Z

No.

ilanbiala · 2016-07-17T05:56:15Z

@lirantal I think you were working on something with grunt-wiredep, can you take a look at this too?

lirantal · 2016-07-17T21:51:49Z

@ilanbiala indeed, thanks for the heads up.
@shanavas786 can you please take a look at my open issue related to this one? #1398. The fix should be simpler, and address gulp more specifically.

shanavas786 · 2016-07-18T06:03:55Z

@lirantal Well, this PR doesn't do much with gulp-wiredep. It just removes the trailing commas created by wiredep from asset config files which causes lint errors (#1360).

This PR primarily addresses grunt-wiredep. Since that also adds trailing commas, grunt-rtc is used to remove them.

lirantal · 2016-07-18T08:37:07Z

@shanavas786 first of all I sincerely appreciate the effort in your PRs.
I don't want to add more dependencies to the project than what is really needed which is why I don't want to continue the route this PR is taking.

Both gulp-wiredep, and grunt-wiredep aren't really doing a lot, just take a look here: https://github.com/stephenplusplus/grunt-wiredep/blob/master/tasks/wiredep.js, we can easily just add wiredep itself and use it in either gulp or grunt.

As to the trailing commas, it sounds like we have some bug there that needs to get resolved. It doesn't make sense that we need a package to remove trailing commas.

So getting back to the point - can you update this PR to take my comments above into consideration and this way to solve both #1360 as well as #1398

shanavas786 · 2016-07-18T14:31:41Z

@lirantal sounds cool.
Had a look at gulp-wiredep too. it has only two lines of code in it. So both gulp-wiredep and grunt-wiredep dependencies can be removed easily.

For handling trailing commas, it needs a little parsing using esprima. Where it would be appropriate to place that piece of code ?, in {gulp,grunt}file.js or in a dedicated file?

lirantal · 2016-07-18T17:46:54Z

You totally understood my points, great :)

As for the commas, why does it even happen? do other projects need to forcely remove the trailing commas in their build process? This sounds like something we aren't doing right in the build process.

shanavas786 · 2016-07-19T06:15:43Z

@lirantal ok, let me explain, basically wiredep is for updating bower dependencies in html source.
It just replaces

<body>
  <!-- bower:js -->
  <!-- endbower -->
</body>

with something like

<body>
  <!-- bower:js -->
  <script src="bower_components/jquery/dist/jquery.js"></script>
  <script src="bower_components/angular/angular.js"></script>
  <!-- endbower -->
</body>

The replacement template, though it is customizable, is uniform for all dependencies (in this case <script src="path-to-bower-dep"></script>). Since we are not directly wiring to html source, but to a javascript array object for later use, we use a replacement template which may look like path-to-bower-dep, with a trailing comma see gulpfile.

The trailing comma is necessary in replacement as it ends up as an array element, and since the replacement template is uniform, wiredep can't handle the trailing comma of last element specifically.

There comes need for a remove-comma task.

shanavas786 · 2016-07-19T08:22:26Z

@lirantal Just one more thought.
I think its better to wire https://github.com/meanjs/mean/blob/master/modules/core/server/views/layout.server.view.html directly. It also removes the dependency on server for a task that could be performed statically.

lirantal · 2016-07-21T13:45:28Z

So to be honest I think that the trailing comma here is still just a symptom of the project not managing correctly the assets to wire. I suggest we do the following in this order too:

First let's get rid of the security issues with the gulp/grunt wiredep modules and work only with the wiredep library
I'm not sure the config/assets/production.js assets file is actually set up correctly because changing it means that we need to commit things to the repository. Let's leave this issue aside for a sec.
Since this is really just a linting issue how about we add a lint ignore statement on that file?

If we're still not in agreement on this then let's start with a PR on (1) to resolve the security issue and figure out how to solve (3) in a later PR.

shanavas786 · 2016-07-21T14:47:14Z

👍 for 1.

3 would only be a temporary fix. The proper solution would be to wire the template file directly and remove the bower dependencies from asset files. That will fix 2 too.

lirantal · 2016-07-21T15:35:06Z

Great. So let's proceed with a PR for (1) and I'll be happy to review your proposed solution for (3) in another PR.

ilanbiala · 2016-07-21T17:56:09Z

SGTM too.

Fixes meanjs#1398

Fixes meanjs#1402

lirantal · 2016-07-23T12:09:47Z

gruntfile.js

@@ -228,7 +257,6 @@ module.exports = function (grunt) {

  // Load NPM tasks
  require('load-grunt-tasks')(grunt);
-  grunt.loadNpmTasks('grunt-protractor-coverage');


@shanavas786 why are we removing this?

lirantal · 2016-07-23T12:11:00Z

@shanavas786 looks like we're done there? just see my comment on the protractor task removal.
can you also build it for production and test it is working well?

shanavas786 · 2016-07-23T12:42:37Z

@lirantal load-grunt-tasks package in require('load-grunt-tasks')(grunt); loads all the grunt tasks. So need not load grunt-protractor-coverage task explicitly.
All the development, test and production builds were successful with each of grunt and gulp :).

lirantal · 2016-07-23T15:24:33Z

You're right. That's great.
Super job making this pokemon happy! :)

Shanavas M added 4 commits July 16, 2016 15:55

Fix(grunt): remove redundant grunt task loading

5b6a1c6

remove explicit grunt task loading for `grunt-protactor-coverage` as `load-grunt-tasks` handles it

Feat(grunt): add wiredep task for grunt

2984208

Automatically update bower dependencies.

Fix(grunt-wiredep):Remove trailing comma from assets

07ccc35

Remove trailing comma from assets configuration generated by wiredep

Fix(gulp): Remove trailing comma from asset files

8554310

Remove trailing comma from asset configurations generated by wiredep

ilanbiala reviewed Jul 16, 2016
View reviewed changes

Fix(npm): use tilde ranges

5cdf216

lirantal self-assigned this Jul 17, 2016

lirantal added this to the 0.5.0 milestone Jul 17, 2016

lirantal added issue:bug need:review+1 platform:build labels Jul 17, 2016

shanavas786 force-pushed the grunt-wiredep branch from cd5dca7 to 5cdf216 Compare July 22, 2016 11:48

shanavas786 added 3 commits July 22, 2016 14:53

Fix(gulp): use wiredep instead of gulp-wiredep

375e762

Fixes meanjs#1398

Fix(grunt): use wiredep instead of grunt-wiredep

7cd674b

Fix(lint): suppress eslint errors in asset files

d9f5ca4

Fixes meanjs#1402

lirantal reviewed Jul 23, 2016
View reviewed changes

lirantal merged commit 0934f87 into meanjs:master Jul 23, 2016

shanavas786 deleted the grunt-wiredep branch July 23, 2016 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feaure(grunt): Wiredep task for grunt #1402

Feaure(grunt): Wiredep task for grunt #1402

shanavas786 commented Jul 16, 2016

ilanbiala Jul 16, 2016 •

edited

Loading

shanavas786 commented Jul 17, 2016

ilanbiala commented Jul 17, 2016

lirantal commented Jul 17, 2016

shanavas786 commented Jul 18, 2016

lirantal commented Jul 18, 2016

shanavas786 commented Jul 18, 2016 •

edited

Loading

lirantal commented Jul 18, 2016

shanavas786 commented Jul 19, 2016

shanavas786 commented Jul 19, 2016

lirantal commented Jul 21, 2016

shanavas786 commented Jul 21, 2016

lirantal commented Jul 21, 2016

ilanbiala commented Jul 21, 2016

lirantal Jul 23, 2016

lirantal commented Jul 23, 2016

shanavas786 commented Jul 23, 2016

lirantal commented Jul 23, 2016

Feaure(grunt): Wiredep task for grunt #1402

Feaure(grunt): Wiredep task for grunt #1402

Conversation

shanavas786 commented Jul 16, 2016

ilanbiala Jul 16, 2016 • edited Loading

Choose a reason for hiding this comment

shanavas786 commented Jul 17, 2016

ilanbiala commented Jul 17, 2016

lirantal commented Jul 17, 2016

shanavas786 commented Jul 18, 2016

lirantal commented Jul 18, 2016

shanavas786 commented Jul 18, 2016 • edited Loading

lirantal commented Jul 18, 2016

shanavas786 commented Jul 19, 2016

shanavas786 commented Jul 19, 2016

lirantal commented Jul 21, 2016

shanavas786 commented Jul 21, 2016

lirantal commented Jul 21, 2016

ilanbiala commented Jul 21, 2016

lirantal Jul 23, 2016

Choose a reason for hiding this comment

lirantal commented Jul 23, 2016

shanavas786 commented Jul 23, 2016

lirantal commented Jul 23, 2016

ilanbiala Jul 16, 2016 •

edited

Loading

shanavas786 commented Jul 18, 2016 •

edited

Loading