Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Grunt #14

Closed
cfarm opened this issue Mar 2, 2015 · 14 comments
Closed

Grunt #14

cfarm opened this issue Mar 2, 2015 · 14 comments

Comments

@cfarm
Copy link
Contributor

cfarm commented Mar 2, 2015

How are we using it, how can we use it better + more deliberately?

Initial discussion

@cfarm
Copy link
Contributor Author

cfarm commented Mar 2, 2015

We're not going to convert existing projects to gulp in a hurry, so maybe we can bookmark that discussion and focus on Grunt and how best to use it.

@jimmynotjim
Copy link
Contributor

  • The rest of the Grunt section above the tasks I linked to is also pretty great at explaining Grunt and the main tasks for people new to Grunt (humble brag :) https://github.com/wp-evolved/evolved-theme#getting-started-with-grunt
  • As far as Gulp goes, I'm all for it but changing every project all at once is probably not wise. I've found it best to change one project, learn from that, then another, then another. After three projects most of the issues/quirks/best practices would work themselves out and we could standardize across all projects (this is coming from a job where we had a crazy number of sites and did just this before I left).
  • We've discussed cleaning up the FJ Gruntfile, I'll be sure to post back here if/when we do.

@ascott1
Copy link
Member

ascott1 commented Mar 3, 2015

Outside of Grunt vs. Gulp (or even using a Makefile), there are a few specific capabilities that I think we should be using in all of our projects:

  • Compile Less into a single stylesheet on watch, but only when CSS changes
  • Compile JS modules on watch but only when JS changes(right now, my preference is that it's done with CommonJS syntax and Browserify)
  • Code linting which will run in a Travis job (at the least) but could also be on watch (developer preference)
  • Run tests as a travis job and pre-commit
  • Minify CSS and Less files for production
  • Optimize images for production

Are there any other core needs?

@anselmbradford
Copy link
Member

Compile Less into a single stylesheet on watch, but only when CSS changes
Compile JS modules on watch but only when JS changes(right now, my preference is that it's done with CommonJS syntax and Browserify)

A scenario that may come up (that I know at least flapjack wants) is page-specific JS and possibly styles, so all site-JS and CSS isn't loading per page. I'm not sure if you want to include that scenario here, but in that case there wouldn't necessarily be a single stylesheet/js file.

Run tests as a travis job and pre-commit

Pre-commit could be developer preference as well? Tests can take a loooong time and sometimes it's preferable to push code to Travis and continue working while the tests run remotely. On commit could be really annoying. If this is happening on a git-hook, pre-push would be better I think (if that's possible with git-hooks—I don't know).

@ascott1
Copy link
Member

ascott1 commented Mar 3, 2015

A scenario that may come up (that I know at least flapjack wants) is page-specific JS and possibly styles, so all site-JS and CSS isn't loading per page. I'm not sure if you want to include that scenario here, but in that case there wouldn't necessarily be a single stylesheet/js file.

This is a good approach. We can certainly bundle common site wide files and also bundle section specific ones.

Pre-commit could be developer preference as well? Tests can take a loooong time and sometimes it's preferable to push code to Travis and continue working while the tests run remotely. On commit could be really annoying. If this is happening on a git-hook, pre-push would be better I think (if that's possible with git-hooks—I don't know).

Pre-commit is what I typed out, but yes, I agree that could be slow and annoying. Pre-push and/or Travis is more what I had in mind.

@anselmbradford
Copy link
Member

Grunt tasks should probably be implementation-agnostic. For example, I'm working with JS linting right now, and the linter is ESLint. I have a task called eslint right now, which I named as such because I was replacing a task called jshint. However, thinking about that further, this is probably the wrong approach and the grunt tasks should describe what they do, not what implementation they run. For example, the task should probably be something like lintjs, for "linting javascript` and then projects can have a consistent grunt command but behind the scenes could be using JSHint, JSLint, or ESLint, without requiring a change to the project documentation if the underlying linter is changed.

If its important to specify the implementation (like if for some reason the project required two linters), the grunt tasks could use subtasks. E.g. lintjs:jshint, lintjs:eslint, etc.

The same argument could be made for things like the browserify task, which describes the implementation, not the task. Instead this should probably be something like bundlejs with the subtask of bundlejs:browserify.

Thoughts?

@jimmynotjim
Copy link
Contributor

👍 standardizing to lintjs makes it easier to start any project and not care what tool it's using, yet automatically know how to run it.

@anselmbradford
Copy link
Member

So digging into this a little deeper I realized it's kind of set up like this already. For example, in the FlapJack gruntfile, there is this line:

grunt.registerTask('test', ['jshint']);

So the task is describing what it's doing (test), and running the implementation (jshint). What's missing from this is that test is not very granular and could be running a whole bunch of things. The grunt-contrib-jshint plugin adds the task jshint, so that grunt jshint works on its own outside of grunt test. If we wanted grunt lintjs, we could approach it by adding an additional task, like:

grunt.registerTask('lintjs', ['jshint']);
grunt.registerTask('test', ['lintjs']);

The annoying thing here is now there are three tasks grunt test, grunt lintjs, and grunt jshint. What if we only wanted grunt test and grunt lintjs, and perhaps a sub-target task of grunt lintjs:jshint?

We could remove the need to specify an implementation in the task list altogether with:

grunt.registerMultiTask('lintjs', 'Lint the JavaScript', function(){
  grunt.config.set(this.target, this.data);
  grunt.task.run(this.target);
});
grunt.registerTask('test', ['lintjs']);

I wish I could get this to be more concise, but even though it's more verbose, it would eliminate differences in this part of the gruntfile if two different projects used different linters. With the grunt config I'll show in a moment this would allow a general grunt lintjs command, as well as specific implementation commands, grunt lintjs:eslint or grunt lintjs:jshint, or both if there was some reason that having both was needed.

The actual config for this (in grunt.initConfig) would look like (for ESLint):

lintjs: {
  eslint: {
    ...
  }
}

Or if you wanted both ESLint and JSHint available:

lintjs: {
  eslint: {
    ...
  },
  jshint: {
    ...
  }
}

@jimmynotjim
Copy link
Contributor

Another reason I love Gulp, task names aren't tied to plugins and it's as easy as

gulp.task( 'lint:src', function() {
  var _src = [
    _src_dir + '/js/**/*.js',
    '!**/libs/**/*.js'
  ];
  var _options = {
    lookup: _src_dir + '/.jshintrc'
  };

  gulp.src(_src)
    .pipe( $.jshint(_options) )
    .pipe( $.jshint.reporter('default') )
    .on( 'error', errorAlert );
});

You can swap out your linting plugin and the task itself stays the same.

@anselmbradford
Copy link
Member

Reading up on grunt best practices I found http://www.thomasboyt.com/2013/09/01/maintainable-grunt.html, which talks about breaking up grunt config files. This can be achieved with https://github.com/firstandthird/load-grunt-config. I kind of like this approach, as the huge Gruntfiles we have are hard to parse. Breaking them apart means updates can be on a per-task basis, without a superficially opaque diff for the whole Gruntfile whenever there's a change. This also means projects can have identical Gruntfiles, but still differ in their implementation of tasks (if desired). For instance, one project could concatenate files different from another project, but they could both have a concat.js task that was loaded into identical Gruntfiles. Any strong opinions on this?

@Scotchester
Copy link
Contributor

@anselmbradford
Copy link
Member

@Scotchester Hah! So let me re-phrase... Any strong opinion about offloading the implementation of config loading to load-grunt-config?

@Scotchester
Copy link
Contributor

I would be all in favor of it 👍

@ascott1
Copy link
Member

ascott1 commented Jul 17, 2015

Discussed 07/17 in front-end meeting. Closing issue.

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

No branches or pull requests

5 participants