I forgot about this, but another reason you might need to pass in the taker instance is in the case of a gulp registry where you probably need access to src, dest and watch.

from undertaker.

I'm noticing a bunch of issues related to this, many are related to the metadata generated and saved inside undertaker to keep the registries simple. I'm not sure how to tackle this yet, but it does need to be addressed.

from undertaker.

@webdesserts I have a potential solution, but I really don't like the API.

function CustomRegistry (taker, options) {
  DefaultRegistry.call(this)
  taker.set('task1', function () {/* .. */})
  taker.set('task2', function () {/* .. */})
  taker.set('task3', taker.parallel('task1', 'task2'))
}
utils.inherits(CustomRegistry, DefaultRegistry)

Notice how the registry never calls this.set and instead it calls taker.set which would allow undertaker to track the metadata for task names and dependencies.

The undertaker API for setting these would need to change to undertaker.registry(CustomRegistry, { custom: 'options' }) which would then new the CustomRegistry internally, passing this and the options to the constructor. Do you have any thoughts on this API? Any way to make it better? A big problem with this is that the new tasks are registered on the old registry before the new registry is set as the registry to use. After, they will still be reassigned through the transfer process but that causes a double registration and they might have already overridden previous tasks with the same name 😦

from undertaker.

Another thought came to my mind. What if the transferring weren't left up to undertaker, but were deferred to the custom registries? It could be implemented in the DefaultRegistry so it would automatically happen if a custom registry inherits properly or they could implement their own transfer logic if they don't want to inherit. Thoughts?

from undertaker.

👍 DefaultRegistry inheritance jazz

from undertaker.

I guess my question would be, If we did start passing in the taker instance, what benefits would we be gaining over the common "just pass the object" plugin pattern?

var registry = function (options) {
  return function (taker) {/* ... */}
}

taker.use(registry({custom: "options"}))

The main benefit I see out of the current pattern is Registry inheritance. Moving the transfer logic sounds promising, though it would be nice to see what that API might look like. How would that handle things like parallel & series? Would a temporary parallel/series be defined on the Default Registry? Would we just be using Bach?

from undertaker.

@webdesserts your sample code might have given me a really interesting idea. Let me flesh it out a bit before I present it here.

from undertaker.

@webdesserts here's what I am thinking. It basically adds a wrapper function to allow options be passed in (like you suggested)

function configRegistry(options){

  class CustomRegistry extends DefaultRegistry {
    constructor(gulp){
      function clean(cb){
        del(options.dist, cb);
      }

      function js(){
        return gulp.src(options.src)
          .pipe(concat('build.js'))
          .pipe(gulp.dest(options.dist))
      }

      var build = gulp.series(clean, js);
      build.displayName = 'build';

      gulp.task(clean);
      gulp.task(js);
      gulp.task(build);
    }
  }

  return CustomRegistry;
}

This would allow undertaker to new up the registry and pass this in as the only argument but still allows the end user to pass in a config object.

This also allows the API to stay exactly the same gulp.registry(CustomRegistry(options)) with only a tiny wrapper overhead for registries that require configuration options.

I also played around with the idea of just using plain functions instead of inheritance but I don't think it looks as good. See below:

function configRegistry(options){

  function customRegistry(gulp){

    function clean(cb){
      del(options.dist, cb);
    }

    function js(){
      return gulp.src(options.src)
        .pipe(concat('build.js'))
        .pipe(gulp.dest(options.dist))
    }

    var build = gulp.series(clean, js);
    build.displayName = 'build';

    gulp.task(clean);
    gulp.task(js);
    gulp.task(build);

    // these methods would replace the methods on the default registry instance
    return {
      get: get,
      set: set
      tasks: tasks
    };
  }

  return customRegistry;
}

from undertaker.

@phated 👍 I definitely like the first one. It's nice and clean.

from undertaker.

Warning: I'm a bit scatter-brained right now. Read at your own risk

I've been thinking about this and one of the problems I see with wrapping the Registry with a function like that, is that it would make extending the registry a bit more difficult. This might be something that can be worked around with a few conventions for exposing the original class, but I honestly wasn't sure how useful extending a Registry other than the default might be so I played around with the idea a bit today.

Musings on Registry Inheritance

The first initial thought I had would be that it would probably be hard to reuse portions of a pipe sequence and insert your own plugins at specific points in that pipeline. For example

class BuildRegistry extends DefaultRegistry {
  constructor (gulp) {
    gulp.task(function build () {
      return gulp.src('src/*.js')
        .pipe(concat('alchemist.js'))
        .pipe(gulp.dest('dist/'))
        .pipe(uglify())
        .pipe(rename({suffix:'.min'}))
        .pipe(gulp.dest('dist/'))
    })
  }
}

class BabelRegistry extends BuildRegistry {
  constructor (gulp) {
    gulp.task(function build () {
      return gulp.src('src/*.js')
        .pipe(named())
        .pipe(webpack({
          output: {
            library: library_name,
            libraryTarget: "umd"
          },
          module: {
            loaders: [ { test: /\.js$/, loader: 'babel-loader?optional=runtime'} ]
          }
        }))
        .pipe(uglify())
        .pipe(rename({suffix:'.min'}))
    })
  }
}

If everything is in the constructor, then we have no way of overriding parts of the logic and making the Registry our own. We can work around that by moving parts of the pipeline into their own methods.

class BuildRegistry extends DefaultRegistry {
  constructor (gulp) {
    gulp.task(function build () {
      var source = gulp.src('src/*.js')
      var bundled = this.bundle(source).pipe(gulp.dest('dist/'))
      var minified = this.minify(bundled).pipe(gulp.dest('dist/'))
      return minified
    })
  }

  bundle (src) {
    return src
    .pipe(concat('alchemist.js'))
  }

  minify (src) {
    return src
    .pipe(uglify())
    .pipe(rename({suffix:'.min'}))
  }
}

class BabelRegistry extends BuildRegistry {
  constructor (gulp) {
    super(gulp)
  }

  bundle (src) {
    return src
    .pipe(named())
    .pipe(webpack({
      output: {
        library: library_name,
        libraryTarget: "umd"
      },
      module: {
        loaders: [ { test: /\.js$/, loader: 'babel-loader?optional=runtime'} ]
      }
    }))
  }
}

Pipeline Composition

What makes this possible is the ability to compose parts of the pipeline together in the constructor. I feel that pipeline composition would make developing Registries pretty fun, but I also feel that it's not very elegant at the moment. Passing the stream around in variables isn't very easy to follow once you start dealing with larger and more complex pipelines. I haven't seen any examples of other people trying to compose pipelines like this (maybe that's should be a sign to me) so I thought I'd throw out a few ideas on how one might approach this pipeline composition problem, specifically in the context of Registry Inheritance, and I'd like to hear your thoughts.

Composing with Functions

Again, we can just use a normal function that takes a stream, pipes it through some plugins and returns it:

function minify (src) {
  return src
  .pipe(uglify())
  .pipe(rename({suffix:'.min'}))
}

To use something like this you would have to "compose" the functions together:

gulp.task(function build () {
      var source = gulp.src('src/*.js')
      var bundled = bundle(source).pipe(gulp.dest('dist/'))
      var minified = minify(bundled).pipe(gulp.dest('dist/'))
      return minified
})

And again, this isn't my favorite.

Composing with _.flow()

I, embarrassingly enough, didn't think of this until I used to word "compose", but you could also use something like lodash.flow(). I haven't really fleshed out what that would look like, but I'll drop what I got here anyway

// convenience wrappers

function src () {
  // not sure if this is right
  return gulp.src.bind(gulp, arguments)
}

function dest () {
  var args = arguments
  return (src) => src.pipe(gulp.dest.apply(gulp, args))
}

gulp.task('build', _.flow(
  src('src/*'),
  bundle,
  dest('dist/'),
  minify,
  dest('dist/'),
))

Composing with Promises

A fancier (and probably insane) way you could maybe approach this is with Promises that pass the gulp stream around.

gulp.task(function build () {
  return src('./alchemist.js')
  .then(BabelToUMD('Alchemist'))
  .then(dest('./dist'))
  .then(minify)
  .then(dest('./dist'))
})

function src () {
  var args = arguments
  return new Promise(function () {
    return gulp.src.apply(gulp, args)
  })
}

function dest () {
  var args = arguments
  return function dest (src) {
    return src.pipe(gulp.dest.apply(gulp, args))
  }
}

function minify (src) {
  return src
  .pipe(uglify())
  .pipe(rename({suffix:'.min'}))
}

function BabelToUMD (library_name) {
  return function (src) {
    return src
    .pipe(named())
    .pipe(webpack({
      output: {
        library: library_name,
        libraryTarget: "umd"
      },
      module: {
        loaders: [ { test: /\.js$/, loader: 'babel-loader?optional=runtime'} ]
      }}))
  }
}

For me, this is the most readable one. The problem with this right now is that I'm pretty sure async-done won't look for a stream in the return value of a promise and test for it's end, so this would have to be added in (if that's even possible). I'm also unsure of what this would mean for error handling.

I've heard a good bit about Observables, but have not checked them out yet, I'm sure a similar solution could be made with them as well? It would at the very least be interesting to see.

Final Thoughts

I'm actually probably going to use a similar solution on my current project with or without Registries. I think composing pipelines like this is extremely useful and is a nice way to keep plugins small while still being able to create larger reusable chunks of pipeline logic. I feel like pipeline composition is a middle ground between plugins and Registries. This also solves a problem for me that I've had with tasks in the past: they can't take parameters (without integrating the cli).

What are you're thoughts on pipeline composition like this? Is there a better solution out there? Is there a better way that you can think of to make Registries extensible without pipeline composition? Or is Registry Inheritance something you would even want users to do (past the Default Registry)?

from undertaker.

Sorry for all the posts, but this one is a bit more on topic.

Another way you could approach the problem of passing in both the config and gulp is to make initialization of a Registry a two step process similar to how Twitter Flight handles their components.

class DefaultRegistry {
  constructor (config) {
    this.config = config
  }

  attachTo(gulp) {
    // do any setup logic here
    this.initialize(gulp)
  }
}

gulp.register = function (registry) {
  registry.attachTo(this)
}

// what the Registry Developer would see (just use initialize instead of constructor)
class BuildRegistry extends DefaultRegistry {
  initialize (gulp) {
    var buildDir = this.config.buildDir || 'dist/'
    gulp.task(function build () {
      console.log('building to', buildDir)
    })
  }
}


// what the consumer would see
gulp.register(new BuildRegistry('app/public/'))

This would allow us to keep our Registry classes closure free and leave Registry configuration as a one step process for the consumer. It's also a bit more readable than the plugin pattern I suggested earlier

from undertaker.

@webdesserts you are amazing. I love the initialize (or maybe init) API. It is so clean and allows the end user to constructor their registry with options but all the task transferring can be done before initialize is called. 🎉

However, I think your other post is taking inheritance to the extreme. The idea behind registries was to share common functionality (e.g. tasks across many common repos inside your company) or to add custom functionality that a minority of users were asking for (e.g. binding a this context to every task). I like to view inheritance in the same light as the team behind React: only inherit a very small API from a base class (React.Component in their case and DefaultRegistry in this case) and don't inherit from more complex child classes (inheriting from a child React component is viewed as very bad). I'm not going to be able to stop people from doing the things you suggested, but hopefully it can be discouraged and undertaker can provide an API that is simple and covers enough use cases that inheritance isn't abused.

from undertaker.

@phated Glad I could help :) Most of the stuff you do is well over my head, so it's nice to be able to help where I can.

I was kinda expecting that to be the case with the first post. It interested me and I just wanted to tread the thought-path even if I knew it would probably be a dead end. I'll probably still look into ways to split up my gulp pipeline though. That's still got my interest peaked at the moment. However, I think that's well into gulp territory and a little off topic for this issue.

from undertaker.

@webdesserts can you take a look at 21cafaa and maybe even link it into any projects where you are using custom registries? I'd like a sanity check before releasing this.

from undertaker.

@phated Will do! I might be busy this weekend and I'll definitely be busy later next week. So I'm a little bit crunched for time, but I'll try it out as soon as I can.

from undertaker.

Sorry to intrude here, but I wanted to add my grain of salt on this. Today I was trying to write a custom registry with the idea of having a common place for shared tasks on our organization. I was facing this very same problem, when I stumbled upon this issue on github.

I have tried the registry-init branch and though I'm not a big fan of the proposed API, I can say it works pretty well. At least, in my case it did, and it provided a solution. Here is my take on why I'm not entirely sold on it:

• It forces you to follow some OOP conventions on your Javascript. This is not a bad thing per se. Coming from a JAVA background, I actually endorse it. But then again, you can't write a custom registry if you don't read and follow the docs thoroughly. And even then, it's not entirely intuitive what you are doing. IDEs don't really help you here, either.
• It feels as if it represents too much boilerplate for such a simple thing.
• It adds an unnecessary additional dependency to util in your project. I know you are not obligated to inherit from DefaultRegistry (and even if you do, you can achieve the same functionality without util). But if you don't, you'll need to write some additional functions that seem to be more private/internal to the framework than part of an API. More so if the official documentation shows the example like that. Copy-and-paste programming is an evil thing.
• Talking about inheritance: if you accidentally misplace the util.inherits line and put it below the init override, then your object loses the function and nothing works. This sounds obvious. It's not. I learned that the hard way.

Overall, my thoughts when using the implementation revolve around: "This does the job... but it feels somewhat hack-ish". Unluckily, I don't have the know-how of the inner works of gulp or undertaker to proposed something concretely different, but perhaps I can contribute from a more conceptual, end-user side, if required.

My two cents.

from undertaker.

@phated Sorry its taking so long to get back to you. I'll be looking at this tonight.

from undertaker.

@phated I created a quick repo to try things out. For some reason series was only running the first task. Is this normal (I'm not used to running things directly from undertaker)?

from undertaker.

In gulp4, sync tasks are being removed. You have to pass in and call a callback

from undertaker.

@webdesserts I had always intended on adding a "hey, your task hasn't finished, is it sync?" debug warning but I had forgotten all about. Maybe I need to look into that

from undertaker.

@nfantone always open to API suggestions before we ship a 1.0 of this library

from undertaker.

For anyone looking at this thread, please note that custom registries are supposed to be an advanced feature of gulp4 (probably the most advanced)

from undertaker.

@phated oh interesting that will be quite the change. Do you have where the discussion on that is?

from undertaker.

Nope, there was multiple and probably some in IRC. Magic sync is bad

from undertaker.

@webdesserts 1 thread I could find while on my phone - robrich/orchestrator#29

from undertaker.

And @nfantone's, I agree that it is a little awkward right now (though probably less awkward than the other solutions). I think as es6 is adopted and the class syntax takes hold it will be much more user friendly (btw gulp has babel support now! yay!). So I think it's worth it to wait out some of that awkwardness. Also, JavaScript isn't really much of an IDE friendly language to begin with. Until types/traits are implemented, I'm not sure that an IDE learnable API should really be a goal for most js libs (especially for plugins).

@phated Thanks for the link, I'm going to test a few things with a custom default registry tonight and I'll give you a summary of my thoughts.

from undertaker.

@webdesserts any thoughts?

from undertaker.

@nfantone always open to API suggestions before we ship a 1.0 of this library

@phated What about providing a convenience function on Undertaker (or even Gulp) that just receives a callback with the taker instance as an argument? Something along these lines:

gulp.register(function (taker) {
    taker.task('foo', taker.parallel(..., taker.series(...)));
    // ...
});

I believe this is similar to some other previous ideas in this thread. But now, you can think of that callback function as, essentially, the proposed registry.init method. Internally, we could then wrap this function on a CustomRegistry instance and call taker.registry with it. By doing this:

• IMHO, it's already much cleaner and simpler.
• It feels much more in tune with the already existent gulp API.
• It encapsulates all off that DefaultRegistry inheritance jazz, without actually losing all that jazz.
• It addresses (almost) all the issues I have pointed out before, including the addition of unnecessary dependencies that are now kept internally.

I can see this concept extended even further, with something like a builder look-a-like:

gulp
    .registry()
    .register('foo', function(taker) {
        return taker.series(..., taker.parallel(...,)));
    })
    .register('bar', function(taker) {
        return taker.series(..., taker.parallel(...,)));
    })
    .create();

I think this is self explanatory. Of course, there would have to be a way of preventing the client from calling taker.task on that what I have called taker above.

from undertaker.

@phated Thanks for the bump.

I played around with it some more and I like the interface. I won't lie, past the "global config" example I couldn't come up with too many ideas of what you would do with a custom DefaultRegistry. The thought of providing helper functions for Registry developer did come to mind. I could easily imagine someone smarter than me taking this and doing something cool with it, though.

If you think the inheritance jazz is worth the extra lines of code, then I say go for it 👍. It wasn't that much of a bother to me and will be a nice clean solution with es6. You've at the very least solved my issues with series and parallel :P

from undertaker.

@nfantone I understand all your points but we don't want to create custom DSLs inside gulp, people can create those as user-land modules, which is where I think the register API you proposed would fit. The get/set methods are super simple in the default registry, so inheritance isn't really required, just suggested.

@webdesserts I actually think I figured out how to use custom registry get/set methods to do forward referenced tasks, something a few people asked for but I don't think it belongs in core. And like I've said before, custom registries are an advanced feature, so I like to expose these power tools to devs.

from undertaker.

Agreed with @phated

as of @webdesserts

oh interesting that will be quite the change. Do you have where the discussion on that is?

Sync functions isn't a issue. You can just wrap them to look like a callbacks and then give them as argument to the .task method

i've created some libs for this
https://github.com/tunnckoCore/always-callback
https://github.com/tunnckoCore/make-callback
https://github.com/tunnckoCore/create-callback

I even can suggest @phated to look at the https://github.com/tunnckoCore/benz idea (i'll release v1 tonight, its ready just final bits and im writing the big big readme), which looks like bach, built on top of his core but handles all types of wanted cases - sync, async, callbacks, generators, returning/handling streams, child processes, promises, generators and etc.
And all that is optional through options. It also support all of functions/tasks in stack to have ne global this context on the fly, plus bonus to pass result to the next in stack (optional). Just readme missing, but tests covers all.

@phated i know your point of co, but with benz you can just provide another generator flow to do the job. Or even other promisify-ing function. I've just put them as disabled defaults - i mean, the are there and will be used only if they are included in dependencies. Please look it, is a cool abstraction on top of same core lego blocks. :)

And thanks for discussion that we had gulpjs/async-done#21, that was definitely the better path. :)
And thanks for the great job on now-and-later and async-done 👍

also can look at https://github.com/tunnckoCore/vez

from undertaker.

i've created some libs for this

actually.. the problem that i face is that promises and generators also looks as sync function, so its hard or too tricky to determine which is sync, which is promise or gen.
in make-callback i've handles generators customly, but not promises.

btw thoughts?

from undertaker.

As it stands, you seem to have your mind pretty made up at this point, @phated. Any chance you could merge the features to master?

from undertaker.

@tunnckoCore After reading through the page @phated linked I didn't really have a problem with the removal of sync tasks as I could easily see the magic that would be necessary to make them work, how undesirable that magic would be, and how forcing the asynchronous tasks would encourage consistency anyway (which is why I didn't bring it up again).

from undertaker.