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

deprecate(grunt): Remove Grunt #1175

Merged
merged 1 commit into from Oct 5, 2016
Merged

Conversation

codydaig
Copy link
Member

@codydaig codydaig commented Jan 30, 2016

This PR serves as the PR to remove grunt, but also as the discussion of what needs to be accomplished before this can get merged in. Is there any functionality still in grunt that is not in gulp? I will add todo items left in comments to this description.

Tasks still to be added to Gulp:

@trendzetter
Copy link
Contributor

Isn't it a pity to remove perfectly working code (as far as I used it) while it's replacements have still issues producing a production build?
#1160

Wouldn't it be sufficient for the time being to add a notice that this code is no longer maintained?

@codydaig
Copy link
Member Author

@trendzetter It prevents the framework from getting bloated. Templatecache is not in grunt, it's only in gulp. We've been adding things to gulp lately in favor or grunt. Grunt is still in the 0.4 releases.

@mg1075
Copy link

mg1075 commented Jan 31, 2016

Agree with @trendzetter; I would vote for continued grunt support.

I don't see where gulp covers the copy:localConfig task.

mean/gruntfile.js

Lines 218 to 226 in e140880

copy: {
localConfig: {
src: 'config/env/local.example.js',
dest: 'config/env/local.js',
filter: function () {
return !fs.existsSync('config/env/local.js');
}
}
}

@mleanos
Copy link
Member

mleanos commented Jan 31, 2016

There's a bit more work to do to get Gulp in line with the tasks that exist in our Grunt implementation. This PR is a great starting point to get that conversation going, as Cody said.

Look at all the dependencies we get to remove, when we get rid of Grunt. The more dependencies this framework has, the more likely we'll run into issues with environmental compatibilities. Ive run into quite a bit of issues with Grunt. Not really at all with Gulp.

Again, like Cody said, the 0.4 releases will have Grunt. Even if someone were to install, or upgrade to 0.5.x, they would be able to add Grunt very easily. It would be a matter of a few copy & pastes.

@codydaig I'll help out with getting Gulp ready for this to be merged. I know the tasks pretty well, between the two.

@ilanbiala
Copy link
Member

@codydaig @mleanos I'll help out with the review.

@trendzetter
Copy link
Contributor

@codydaig I am very much in favor of lean and mean. I also value continuity and stability as a user of the framework. It appeared to me that when tracking the upstream I could run into issues with producing occasional production builds.

@ilanbiala
Copy link
Member

@trendzetter what issues have you had with Gulp?

@trendzetter
Copy link
Contributor

I am just referring to #1160
The production build produced by gulp is not usable if I understand correctly. A solution is in the making but not finished.
So far I have been using the grunt scripts to run my project, very recently I started running gulp for development. I can give the production build a try tonight to establish if it breaks currently working functionality or if it only affects "new things".

@ilanbiala
Copy link
Member

@trendzetter that is something that doesn't even exist in our Grunt setup, so that is not an issue really. Please try Gulp and let us know what else needs to be added to allow us to switch over from Grunt to Gulp.

@trendzetter
Copy link
Contributor

I not sure what you mean by "that doesn't even exist in our Grunt setup".
If I run "grunt build" I get a set of minimized files which I use for setting up a demoserver (in preparations of the real production when the application goes live).
I admit I have no clue about buildtools. I will look into what you are saying tonight.

@ilanbiala
Copy link
Member

@trendzetter nothing in Grunt creates a template cache for Angular in the same way that Gulp does. What you are referring to is minification, which both build tools already do.

@trendzetter
Copy link
Contributor

@ilanbiala Sorry for my ignorance. I just tried the minification build and they both run the same at first sight although the grunt dist files are 1kb smaller.

@trainerbill
Copy link
Contributor

Thoughts on using:

https://www.npmjs.com/package/gulp-strip-debug

Would be nice to have some debug statements in the code base. Like on controller instatiation do

console.log('ArticlesController::Init', vm);

Then let gulp strip them on production/test build. Obviously we wouldn't allow all console.logs in PR's, but having strategic ones would save a bunch of time IMO

Thoughts?

@ilanbiala
Copy link
Member

@trainerbill I don't think we have that in Grunt, so that should be in a different issue. This issue is just about getting everything in Grunt into Gulp so we can strip Grunt out.

@mleanos
Copy link
Member

mleanos commented Feb 6, 2016

@codydaig The following tasks can be added to the list

  1. mkdir:upload
  2. ngAnnotate - The Build task uses this

Not sure if we need these. Does anyone have any insight?
3) mongoose
4) node-inspector

@codydaig
Copy link
Member Author

thanks @mleanos

@rhutchison
Copy link
Contributor

@mleanos where are we using ngAnnotate?

@@ -16,7 +16,7 @@
},
"scripts": {
Copy link
Member

Choose a reason for hiding this comment

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

Scripts were already taken care of: 5a91d7d

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I'm gonna close this PR in favor of a new one. Some things have changed.

@mleanos
Copy link
Member

mleanos commented Aug 30, 2016

I'm fine with removing Grunt officially. I had reservations on supporting users that rely on Grunt already. However, as someone pointed out earlier, anyone can look back at previous versions of the framework to see how to integrate Grunt.

@lirantal lirantal self-assigned this Aug 30, 2016
@mleanos
Copy link
Member

mleanos commented Sep 8, 2016

I think we can merge this in, after the conflicts are resolved (re-base). Everything but the ngAnnotate has been added to Gulp. Although, we do include the Gulp plugin for ngAnnotate since it's being used in the minification of the js files. But we don't need a Gulp task dedicated to ngAnnotate.

@mleanos
Copy link
Member

mleanos commented Oct 2, 2016

@codydaig Would you like to wrap this one up? It looks like this is the last key issue before we're ready to release 0.5.0.

@codydaig
Copy link
Member Author

codydaig commented Oct 4, 2016

@mleanos Rebased.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 73.224% when pulling 204ab55d868fdd489af27b79ade605c72f6a4a32 on codydaig:removeGrunt into 287f81c on meanjs:master.

9 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 73.224% when pulling 204ab55d868fdd489af27b79ade605c72f6a4a32 on codydaig:removeGrunt into 287f81c on meanjs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 73.224% when pulling 204ab55d868fdd489af27b79ade605c72f6a4a32 on codydaig:removeGrunt into 287f81c on meanjs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 73.224% when pulling 204ab55d868fdd489af27b79ade605c72f6a4a32 on codydaig:removeGrunt into 287f81c on meanjs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 73.224% when pulling 204ab55d868fdd489af27b79ade605c72f6a4a32 on codydaig:removeGrunt into 287f81c on meanjs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 73.224% when pulling 204ab55d868fdd489af27b79ade605c72f6a4a32 on codydaig:removeGrunt into 287f81c on meanjs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 73.224% when pulling 204ab55d868fdd489af27b79ade605c72f6a4a32 on codydaig:removeGrunt into 287f81c on meanjs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 73.224% when pulling 204ab55d868fdd489af27b79ade605c72f6a4a32 on codydaig:removeGrunt into 287f81c on meanjs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 73.224% when pulling 204ab55d868fdd489af27b79ade605c72f6a4a32 on codydaig:removeGrunt into 287f81c on meanjs:master.

@coveralls
Copy link

coveralls commented Oct 4, 2016

Coverage Status

Coverage remained the same at 73.224% when pulling 204ab55d868fdd489af27b79ade605c72f6a4a32 on codydaig:removeGrunt into 287f81c on meanjs:master.

@codydaig
Copy link
Member Author

codydaig commented Oct 4, 2016

Despite the one build chain failing, the other passes. It also passes when it runs on my fork of mean.

@mleanos I believe this is ready to be merged in.

@coveralls
Copy link

coveralls commented Oct 4, 2016

Coverage Status

Coverage remained the same at 73.224% when pulling 7412100 on codydaig:removeGrunt into 287f81c on meanjs:master.

Copy link
Member

@mleanos mleanos left a comment

Choose a reason for hiding this comment

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

LGTM. Look at all those libraries we're removing 👍

@lirantal
Copy link
Member

lirantal commented Oct 4, 2016

Yay, finally! :)

@codydaig codydaig merged commit 83c7c47 into meanjs:master Oct 5, 2016
@codydaig codydaig deleted the removeGrunt branch October 5, 2016 01:28
@sujeethk
Copy link
Contributor

sujeethk commented Oct 5, 2016

@codydaig qq -Is there a reason why Grunt and Grunt-Cli are left out in the dependencies?

@codydaig
Copy link
Member Author

codydaig commented Oct 5, 2016

@sujeethk See #1543. I must have missed that in the rebase. Thanks for pointing it out!

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.

None yet