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

Organize Gruntfiles by Packages / Features #1048

Closed
scottrippey opened this issue Jan 28, 2014 · 37 comments
Closed

Organize Gruntfiles by Packages / Features #1048

scottrippey opened this issue Jan 28, 2014 · 37 comments
Milestone

Comments

@scottrippey
Copy link

Grunt's config file is organized by tasks.
This makes it very difficult to organize. Even if split into separate task files, it's likely that a single feature will span several tasks, and multiple features will overlap tasks.

The concept of Package by feature, not layer would greatly improve the way we organize our Gruntfile configuration.

@cowboy @stevenvachon @k3n @brian-frichette Thoughts?

@cowboy
Copy link
Member

cowboy commented Jan 28, 2014

Yep, something I've been thinking about.

@scottrippey
Copy link
Author

@cowboy What do you think about the grunt.mergeConfig idea in #1039 ?
I created some usage examples here https://github.com/scottrippey/grunt-config-merge

@k3n
Copy link

k3n commented Jan 28, 2014

@scottrippey Of course I'm going to be all for it!

One thing about your examples though: that's package-by-layer, e.g. grouping components by technology (JS, CSS, etc.) is a layer. To better illustrate the desire (or at least from my POV), would be to have a fooWidget and a barWidget, each of which need to build their own independent CSS/JS/etc. Given that context, each widget would be it's own feature, as opposed to being a layer of your technology stack.

edit: with that said, it does demonstrate how you can configure tasks in multiple different locations, which is the end goal I believe. Perhaps it is PbF after all, just with a different definition for 'feature' than I had originally thought of.

@bfricka
Copy link

bfricka commented Jan 28, 2014

It's a challenge in its current state. Simple, shallow projects work well
with a single file or a task-based division of files. But the task-based
approach buries some of the semantics that describe a particular build
strategy.

While I am vaguely uncomfortable with the mergeConfig implementation, it
provides some much needed flexibility with regard to structuring. (I need
to think more about what prickles me about mergeConfig)

It's definitely a step in the right direction and a discussion worth
having.

@scottrippey
Copy link
Author

@k3n What you said makes perfect sense ... my example is flawed. In my actual application, I've got tasks like "desktop" and "mobile". Those are better examples.

@scottrippey
Copy link
Author

@brian-frichette The reason I like mergeConfig is because it perfectly mirrors initConfig and has a very low learning curve. I hope that deprickles you a little :)

@logankoester
Copy link

My current solution to this (which actually works pretty well) has been to create a subdirectory for each widget containing all its resources (js, css, etc), and put a Gruntfile in each of them. The root application then uses grunt-hub to manage these.

An added benefit of this solution is that it's easy to refactor these widgets out into npm & bower modules, which will then already have a Gruntfile, as opposed to extracting out the widget's build strategy at refactor-time.

A downside, of course, is that most of these Gruntfiles are identical redundant code. I'm using a Yeoman generator to create them.

Edit: Also worth noting, each of my widgets contains a tasks/ directory with the actual configuration split into files. The Gruntfile just tells Grunt to load them.

@bfricka
Copy link

bfricka commented Jan 28, 2014

@scottrippey, Yeah, that's a pretty strong argument :-)

@doowb
Copy link

doowb commented Jan 28, 2014

@scottrippey have you tried this in a situation where you want to do grunt build:desktop and the desktop target is defined in multiple configs that get merged together?

// js config
grunt.mergeConfig({
  concat: {
    desktop: {
      files: [
        { dest: 'scripts.min.js', src: ['**/*.js'] }
      ]
    }
  }
});

// css config
grunt.mergeConfig({
  concat: {
    desktop: {
      files: [
        { dest: 'styles.min.css', src: ['**/*.css'] }
      ]
    }
  }
});

Will the files property get merged? I think that's the thing that feels weird to me, but I like the idea.

@scottrippey
Copy link
Author

@doowb No, I haven't tried that. In fact, I only ever intended to merge 2-levels deep, so I always use unique target names.
For your example, I would create a desktop_js and desktop_css targets, and then do grunt.registerTask('desktop', [ 'concat:desktop_js', 'concat:desktop_css' ]);

Since merge uses lodash.merge, in your example, the 2 "files" arrays would just override each other, not concat. I can see how that would be confusing.

I think it's pretty much impossible to know how to correctly merge duplicate targets. So, perhaps merge is the wrong algorithm. Perhaps it would be better to limit to 2 levels deep, and to throw exceptions when overlapping targets exist. But also, I'd still want to be able to merge default options (eg concat: { options: { ...nested options... } }).

@cowboy
Copy link
Member

cowboy commented Jan 28, 2014

We could easily build the following config in a different way.

grunt.init({
  task1: {
    options: {a: 'task1'},
    target1: {options: {a: 'task1-target1'}},
    target2: {options: {a: 'task1-target2'}},
  },
  task2: {
    options: {a: 'task2'},
    target1: {options: {a: 'task2-target1'}},
    target2: {options: {a: 'task2-target2'}},
  },
  task3: {
    options: {a: 'task3'},
    target1: {options: {a: 'task3-target1'}},
    target2: {options: {a: 'task3-target2'}},
  },
});

Sure, we can set per-task config all at once:

grunt.config.set('task1', {
  options: {a: 'task1'},
  target1: {options: {a: 'task1-target1'}},
  target2: {options: {a: 'task1-target2'}},
});

But, for example, with a method like grunt.config.setByTarget we could build the config a bit differently.

// Normal per-task config, omitting target-specific config.
grunt.init({
  task1: {options: {a: 'task1'}},
  task2: {options: {a: 'task2'}},
  task3: {options: {a: 'task3'}},
});

// Remember that you can specify an array of property name parts like this.
grunt.config.set(['task1', 'target1'], someObject);

// So here's a new helper method. Dunno if it makes sense.
grunt.config.setByTarget = function(targetname, targetConfigs) {
  for (var taskname in targetConfigs) {
    grunt.config.set([taskname, targetname], targetConfigs[taskname]);
  }
};

// Per-target, per-task options, grouped by target.
grunt.config.setByTarget('target1', {
  task1: {options: {a: 'task1-target1'}},
  task2: {options: {a: 'task2-target1'}},
  task3: {options: {a: 'task3-target1'}},
});
grunt.config.setByTarget('target2', {
  task1: {options: {a: 'task1-target2'}},
  task2: {options: {a: 'task2-target2'}},
  task3: {options: {a: 'task3-target2'}},
});
grunt.config.setByTarget('target3', {
  task1: {options: {a: 'task1-target3'}},
  task2: {options: {a: 'task2-target3'}},
  task3: {options: {a: 'task3-target3'}},
});

Just an example... thoughts?

@scottrippey
Copy link
Author

@cowboy I like that it inverts tasks and targets ... targets are like features, so it makes sense. But sometimes a single feature needs multiple tasks, which would be difficult this way.
Another important thing to think about, is that every example and all documentation on every grunt task out there is using the multi task syntax. It took me quite a while to learn the intricacies of that syntax, and I would be hesitant to learn an additional syntax.

@scottrippey
Copy link
Author

@cowboy it's interesting that grunt.config.set can be used to effectively "merge" individual tasks ... can it be used to merge individual targets?

@bfricka
Copy link

bfricka commented Jan 29, 2014

@scottrippey, These were thoughts precisely. @cowboy, the syntax and structure you presented is clever, but obtuse. The fact that mergeConfig mirrors initConfig makes it a no-brainer.

After looking at the implementation (which is what "prickled" me, not the syntax), I am only bothered by the dependency on grunt.config.data. I'd like to see a more closely integrated solution, or at least a publicly defined getter/setter for data. As I mentioned earlier, having grunt.config.get() return all data when called without arguments would be a solid approach. Then you need only refactor grunt.config.set() to also except hash (assuming it doesn't already, I haven't checked, but it appears to be a two-param method only) and you have a more robust basis for the mergeConfig implementation.

@Bartvds
Copy link

Bartvds commented Jan 29, 2014

@scottrippey I have a module that does this kind of thing: gruntfile-gtx. It is a wrapper to use in your Gruntfile with many helpers to creatively build the config object (and some aliases).

It also covers that case where one build 'job' uses multiple plugins that share some data (like paths or id's). My recurring example is running tests for modules of my TypeScript projects: each needs a sequence of clean, typescript, tslint mocha-test on the same base path.

@scottrippey
Copy link
Author

@brian-frichette @cowboy

I think I might have found a solution ... I reimplemented the grunt.mergeConfig function, removed dependencies on grunt.utils._.merge and grunt.config.data, and instead, am using grunt.config.set([ task, target], targetConfig).
https://github.com/scottrippey/grunt-config-merge/blob/tasks-and-targets-only/grunt.config.merge.js
@doowb This will constrain the merge so that it only merges targets onto tasks, but doesn't attempt to deep-merge the targetConfig. If the target already exists, it will be overwritten (but perhaps it would be better to throw an error?).

@bfricka
Copy link

bfricka commented Jan 29, 2014

@scottrippey This is much better. The merge was a weird concept since I can't think of a reason you'd want to do a deep merge. For example if you had a task w/ a files array, and tried to merge two of the same task:target, merge would take the first files array, but add any object properties. It's easy to imagine the sort of weird behavior and bugs that would be introduced by this.

If instead, you are just assembling a variety of targets of task (jshint:desktop, jshint:mobile, etc), you're golden. This was the use case and I think this addresses it well.

@cowboy
Copy link
Member

cowboy commented Jan 30, 2014

@scottrippey can you show me with some code how it works? Show me:

  1. how you'd build an example config using grunt.initConfig
  2. how you'd do the same thing, better, with your grunt.mergeConfig function.

Sell me.

@scottrippey
Copy link
Author

@cowboy All right ... give me a few minutes, and I'll concoct an example that'll blow your mind.

@scottrippey
Copy link
Author

@cowboy I got a bit busy, but I'm delivering on my promise: https://gist.github.com/scottrippey/8720672
I rest my case, your honor.

@k3n
Copy link

k3n commented Jan 30, 2014

That's a great example @scottrippey, thanks for working that up! Sorry I haven't contributed much here, I was pulled in another direction at work and so I had to refocus, but I think you guys are doing a good job of hashing this out.

About the example itself: my use-case would have each of the mergeConfig() calls (except maybe the defaults) live in its own file. I think that's implied here, but wanted to point out that that would be where the real power comes in IMO.

@scottrippey
Copy link
Author

@k3n Yes, the next logical step (which is optional, but fully recommended) is to separate each package into its own file.
For example:

Gruntfile.js
/config/default-options.js
/config/core-js.js
/config/core-css.js
/config/extras-js.js
/config/extras-css.js

@vladikoff vladikoff added this to the 0.5.0 milestone Mar 4, 2014
@scottrippey
Copy link
Author

After a few months of using this pattern, I'm extremely happy with its effect, and I'd really like to see this merged.
I'm certain that Organizing by Package will become the defacto way in which builds are organized, and that mergeConfig will gain a lot of popularity.

Gulp organizes by package, too

This was one of the first appealing things I noticed about Gulp ... by default, Gulp is already "package" oriented, and encourages organizing everything by package.

So @cowboy, I'd suggest you review the following examples, if you haven't been convinced:

Example Use-Case

In the following example, I'm using Grunt to build 4 packages, core-js, core-css, extras-js, and extras-css. It shows how grunt.mergeConfig allows each package's config to be a modular, self-contained block. These blocks can even be extracted to separate files, which really makes this powerful.
https://gist.github.com/scottrippey/8720672

This change is only a few lines of code, it's completely new (non-breaking) functionality, and it makes organization a breeze. The Pull Request #1039 is ready to go.

@cowboy
Copy link
Member

cowboy commented Mar 13, 2014

@scottrippey, while a grunt.mergeConfig method (I'd probably implement it as grunt.config.merge) seems useful, with respect to solving the specific problem of configuring same-named targets across multiple tasks in a single method call, the example in your gist seems unnecessarily verbose.

The API I had already suggested for grunt.config.setByTarget solves the specific problem you're trying to solve, and does it with a simpler data structure. No, the structure isn't exactly what will be merged into the config, but it's a lot less verbose. And FWIW, I had actually completely forgotten about this issue, and came up with the following example after just clicking on the link to your gist in the GitHub notification email I received. (Full disclosure: it was slightly different, I called the method setTarget not setByTarget)

Either way, while a general merge method might be useful, it seems like the larger problem to be solved is that people might want to configure same-named targets across multiple tasks in a single method call. My method attempts to address that specific use-case.

(Also note that in this example, setting default options is done with grunt.initConfig)

module.exports = function(grunt) {

  // Default options
  grunt.initConfig({
    uglify: {
      options: {
        report: 'min',
        compress: { loops: true, unused: true, unsafe: true }
      }
    },
    watch: {
      options: { atBegin: true, livereload: true }
    }
  });


  // Core css
  grunt.config.setByTarget('core-css', {
    less: {
      src: 'core/**/*.less', dest: 'dist/core.css'
    },
    watch: {
      src: 'core/**/*.less', tasks: [ 'less:core-css' ]
    }
  });


  // Core JS
  grunt.config.setByTarget('core-js', {
    concat: {
      src: 'core/**/*.js', dest: 'dist/core.js'
    },
    uglify: {
      src: 'core/**/*.js', dest: 'dist/core.min.js'
    },
    watch: {
      src: 'core/**/*.js', tasks: [ 'concat:core-js', 'uglify:core-js' ]
    }
  });


  // Extras CSS
  grunt.config.setByTarget('extras-css', {
    concat: {
      src: 'extras/**/*.css', dest: 'dist/extras.css'
    },

    watch: {
      src: 'extras/**/*.css', tasks: [ 'less:extras-css' ]
    }
  });


  // Extras JS
  grunt.config.setByTarget('extras-js', {
    concat: {
      src: 'extras/**/*.js', dest: 'dist/extras.js'
    },
    uglify: {
      src: 'extras/**/*.js', dest: 'dist/extras.min.js'
    },
    watch: {
      src: 'extras/**/*.js', tasks: [ 'concat:extras-js', 'uglify:extras-js' ]
    }
  });


  grunt.loadNpmTasks('grunt-contrib-concat');
  grunt.loadNpmTasks('grunt-contrib-jshint');
  grunt.loadNpmTasks('grunt-contrib-watch');
  grunt.loadNpmTasks('grunt-contrib-uglify');
};

@scottrippey
Copy link
Author

I see ... the setByTarget merges into the config object, and allows you to omit the target names. That's good, and I think it could improve organization, and it saves some typing.

However, this seems like it will only work for multi-target tasks, right? If I have a package that requires a basic task (such as nodeunit), or simply needs some data for templates (such as pkg), I couldn't use this syntax, and I'd have to figure out how to use grunt.config.set(...).
It's also possible that a single package, such as core-js, might need 2 targets on the same task. For example, you might want to build both core.js and core.min.js using uglify for both. So I'd need to split my config into 2 setByTarget calls.

So it seems that this achieves a similar effect, but has a few limitations, and comes with a learning curve.

My point is that mergeConfig simply improves initConfig, and has no learning curve, which is why it seems like the right way to go.

@k3n
Copy link

k3n commented Mar 13, 2014

I'm in favor of anything that facilitates the end-goal, but do agree with the approach from @cowboy which requires less cruft. I was actually toying with attempting to implement a similar pattern, but which would instead use the module name as the task target, which would preclude one from needing to explicitly define the task target in the code.

Just for demo purposes, I'll call this imaginary method setByModule():

// core-css.js
module.exports = function(grunt) {
  grunt.config.setByModule({
    less: {
      src: 'core/**/*.less', dest: 'dist/core.css'
    },
    watch: {
      src: 'core/**/*.less', tasks: [ 'less:core-css' ]
    }
  });
};

Here, setByModule() would infer the module name -- core-css -- and then apply the config to that target.

The reason for this is that moving things around should be easier, and it's one less thing that another developer could miss in the implementation. However, I yield that I haven't fully thought this through, and it may not even make sense for some use-cases.

@bfricka
Copy link

bfricka commented Mar 13, 2014

@k3n I can't for the life of me see what "cruft" you're speaking of. I think @scottrippey's points are valid, and would argue that mergeConfig (or config.merge) has less cruft, since it mirrors initConfig

@scottrippey
Copy link
Author

The "cruft" is the fact that initConfig (and therefore mergeConfig) requires you to name your targets. So if a package has 3 different tasks, you'll probably repeat the target name 3 times.

So that's what @cowboy and @k3n are trying to do with setByTarget / setByModule ... specify the target name first, so instead of task > target > config, they're able to do target > task > config, which saves a little typing, aka "cruft".
And in the process, they've implemented their own merge functions; albeit, they've inverted the Grunt config structure.

That's the point of mergeConfig ... allow merging, but keeping the same structure as initConfig.

I hope this helps everyone understand the difference.

@bfricka
Copy link

bfricka commented Mar 13, 2014

I get it, I just don't see it as "cruft". The mirroring of initConfig is anything but crufty. And these other solutions are scarcely improvements in syntactic simplicity whilst adding an additional API to understand.

@k3n
Copy link

k3n commented Mar 13, 2014

@brian-frichette I'm using "cruft" to refer to code that I believe is unnecessary and redundant. The point that @scottrippey's implementation more closely resembles the 'normal' config usage is definitely accurate, but to say that using more code is less cruft is factually incorrect IMO.

@scottrippey's method would require 2 * # of tasks extra LoC, so if you do less, cssmin, then concat, that's 6 extra LoC that are essentially redundant. @cowboy's method removes those 6 extra lines, but still requires you to explicitly state the task target. My version also removes the 6 extra lines, but goes further in removing the requirement to also explicitly provide the task target.

@k3n
Copy link

k3n commented Mar 13, 2014

Also, as I stated before, I'm not even completely sold on my own idea, I just wanted to throw it out there for discussion since we were already on the topic. It came to me when I was implementing mergeConfig for around 12 different task targets, when I missed one or two due to forgetting to change every single reference.

There is currently no mechanism in place to guard against mixing task targets, so the following is completely legal although most likely not what you'd want (notice I've mixed task targets extras-css and extras-js):

// Extras CSS
grunt.mergeConfig({
  concat: {
    'extras-css': {
      src: 'extras/**/*.css', dest: 'dist/extras.css'
    }
  },
  uglify: {
    'extras-js': {
      src: 'extras/**/*.js', dest: 'dist/extras.min.js'
    }
  },
  watch: {
    'extras-js': {
      src: 'extras/**/*.js', tasks: [ 'concat:extras-js', 'uglify:extras-js' ]
    }
  }
});

@bfricka
Copy link

bfricka commented Mar 13, 2014

I see your point, and I think I would agree if this were a different sort of framework. I'm generally not a fan of verbosity, assuming intent can adequately be described otherwise.

However, it's a build tool, and there is already a significant barrier to entry. I have heard this numerous times from people trying to learn Grunt. Sure, once you get it, it's utterly simple, but that's true of many things in programming and life :)

Anyways, no reason to belabor the point. I'm happy that this discussion is occurring.

@stevenvachon
Copy link

@cowboy's setTarget/setByTarget will not work in the case of multiple targets that fit into a single feature. Here's an example using @scottrippey's mergeConfig:

grunt.mergeConfig({
    "mocha": {
        "test-dev": {},
        "test-dist": {}
    }
});

"test" being the feature, but because there are two targets, I couldn't simply name it "test".

Why not just implement both? If over-complication is an argument, then I'd say just go with mergeConfig as it has no learning curve.

@creynders
Copy link

Maybe I'm totally missing something important here, but with the plugin I wrote load-grunt-configs you can do this w/o the need for an extra API method. You do have to put them into separate files though.

@scottrippey
Copy link
Author

@creynders Your plugin looks great, and it definitely achieves organizing by package. Thanks for sharing.
I still see 2 advantages of mergeConfig, though. First, it's more flexible because it doesn't require separate files. Second, the syntax is identical to initConfig, which means there's no learning curve; you can copy-and-paste examples from the internet with only 5 changed characters.
This method does not care how your files are organized, or how to load them ... that's still up to you. It simply enables the ability to break apart your config into separate chunks.

@vladikoff vladikoff modified the milestones: 0.4.5, 0.5.0 Mar 24, 2014
@dallonf
Copy link

dallonf commented Sep 19, 2014

Relevant to this subject: I've been working on a utility library that provides an alternate syntax for Gruntfiles that similarly groups by package and additionally, execution order: https://github.com/dallonf/bacon-grunt

That said, this has a much larger learning curve than mergeConfig and it's not nearly as flexible.

@scottrippey
Copy link
Author

This was, in fact, added to Grunt v0.4.5 -- it includes grunt.config.merge (but not the alias grunt.mergeConfig). So I'm closing this issue. Thanks everyone.

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