Abstracting module definition.

[wbyoung]

Background

I wanted to create a new plugin, gulp-ember-emblem.

There are multiple forks of gulp-handlebars. They seem to have been forked at a different point in time, and they haven't been pulling in changes from from gulp-handlebars as changes have been made. That got me thinking… how can I abstract this a bit so that some of the functionality can change more independently.

I came up with gulp-define-module.

Request

As you'll see from the request, this is a pretty radical change. I'm hoping that we can somehow either collaborate to get this into each of the plugins that forked from the gulp-handlebars repo. And I think this could end up being a better solution for the gulp community as a whole since it will result in more continuity between each of these plugins.

The basic idea here is that module definition, AMD, Node, CommonJS, or just plain browser can be pulled out into its own module. The resulting gulp flow looks something like this:

var handlebars = require('gulp-handlebars');
var defineModule = require('gulp-define-module');

gulp.task('templates', function(){
  gulp.src(['client/templates/*.hbs'])
    .pipe(handlebars())
    .pipe(defineModule('node'))
    .pipe(gulp.dest('build/templates/'));
});

This module also has the possibility of replacing gulp-declare, but I'd probably only recommend that for the simple cases (where you know the namespace already exists).

[buschtoens]

+1 I am totally in favor of this.gulp-handlebars should only output the bare minimum.

[lazd]

gulp-declare does much more than this -- it splits the template name on dots and defines the namespaces required along the entire path. This is a required and very common use case for gulp-handlebars, so we'll need to add back the same example that exists currently in addition to the gulp-define-module.

Can you please re-add that example in your pull request?

[lazd]

I like it. Let's merge this after you address the inline comments. Thanks!

[lazd]

@wbyoung, any thoughts on defining partials in the context of your pull request? In #13, @kahwee used gulp-wrap. I'd like to hear how you think this should be done.

[lazd]

Package names should be wrapped in inline code blocks, not italics.

[wbyoung]

@lazd hopefully that last commit addresses all of the comments you made. Let me know if you'd like anything else changed.

[lazd]

@wbyoung, will review shortly. Should we wait on wbyoung/gulp-define-module#1 to simplify the documentation?

[wbyoung]

@lazd probably not. I may not get to it until sometime next week, so if you have time now, I'd move forward with this. Then we can perhaps get some feedback and start iterating on the changes if need be.

[lazd]

@wbyoung, I think it'll be a documentation issue at the end of the day and won't actually affect the code for gulp-handlebars, so we can move forward without it.

[wbyoung]

Great!

[lazd]

Hmm, defineModule('plain') is only required to add the Handlebars.template call. If wrapped was still an option to gulp-handlebars, then gulp-define-module would not be required at all for this use case. @wbyoung, thoughts?

[wbyoung]

Ahh… just lost the comment I typed out… rewriting.

I had the same thought when I just this example. I see it both ways. I approached this trying to abstract as much as possible from these 3 plugins and I may have been a little overzealous.

The case for putting the wrapper in gulp-handlebars is that it's pretty specific to the plugin and arguably you can't do anything with the output of the plugin if you haven't wrapped it yet. Also, the result is still a function either way.

The case for putting it in gulp-define-module is that it access Handlebars, and as you mentioned before, you'd then need a require in there. Also, it does achieve a little more abstraction. Finally, gulp-handlebars could be seen only as a compiler, so it should handle just that one thing.

Honestly, I think both are "right" and I have no problem with it either way.

[lazd]

Yeah, I think it makes sense to use gulp-define-module or gulp-wrap to add it. We're good here :)

[lazd]

This is looking really good. I'll merge after wbyoung@e2ab0b8 is handled. Feel free to squash your commits and force push back to your branch if you like (not required).

[wbyoung]

I commented on that previous change and why I think it should still be there, but please feel free to overrule here. I also squashed the commits.

[lazd]

Looking pretty good, I have a few minor nitpicks but I'll quickly correct them after merge. Going to check out the branch and pick through it, then will merge. Probably won't have time until tomorrow, though.

Thanks for the hard work!