node-task / spec

A specification for javascript tasks.
MIT License
153 stars 2 forks source link

Overall brainstorm #8

Closed marcooliveira closed 11 years ago

marcooliveira commented 11 years ago

(follow up from https://github.com/IndigoUnited/automaton/issues/49)

Here's a list of topics that me and @satazor sorted out, after analysing the current state of the spec:

Looking forward to get some feedback on this. Sorry for the long post, just wanted to be meticulous.

Also, might be a good idea to exchange Skype/GTalk contacts, so we don't pollute the repository with raw ideas (also speeds up everything).

marcooliveira commented 11 years ago

By the way, forgot to mention, the spec is looking promising ;)

tkellen commented 11 years ago
  1. Docs have been updated to include a spec only.
  2. Task options including defaults are now in the spec.
  3. The spec now requires that run always return a promise.
  4. File handling tasks have been moved to a sub-specification which I have nearly completed and will publish shortly.
  5. I'm comfortable with the name node-task for now, but I see your point and will consider this.

No worries about the long post!

satazor commented 11 years ago

Thanks @tkellen for considering our points. Still there's a problem with the emitter being part of the task object itself. If someone tries to use a task in parallel like this:

var mincss = require('mincss-task');

mincss.run({
  files: {
    'dist/main.css': 'src/main.css'
  }
}).on('log', function () {
  console.log('something was logged when running min of main.css');
});

// And afterwards..
mincss.run({
  files: {
    'dist/panel.css': 'src/panel.css'
  }
}).on('log', function () {
  console.log('something was logged when running min of panel.css');
});

Since the emitter is common to the object itself, log events and others will collide when running the same task in parallel. This will make parallel support at the task runner somewhat difficult, specially if we got more important events that might carry data.

Like we said in the post above, this could be solved with an emitter being returned that would have start, end, log.* and maybe others (data for piping?).

marcooliveira commented 11 years ago

Awesome! Great to see that we see eye to eye in a lot of topics.

@tkellen

  1. The spec now requires that run always return a promise.

Great to see this consistency. But consider the notes below.

@marcooliveira / @satazor:

Why not simply rely on the already in spec emitter, and specify a set of standard events (something like start and end, possibly a few more), that will provide feedback about what's happening? Also, a new emitter should always be created and returned when invoking run, so there is no collision between re-used tasks. (imagine using the same task twice, in parallel, would cause a conflict in the listeners, if there weren't two emitters).

I'm just wondering what's the reasoning behind using promises instead of the emitter. I see that the idea of using emitters or promises has been kicked around in other issues, but just wondering why the emitter did not stick. At first glance, seems like a powerful solution. /cc @millermedeiros @sindresorhus


I'm comfortable with the name node-task for now, but I see your point and will consider this.

Cool. If this is to happen, probably better sooner than latter, less hassle. Maybe move that into a separate issue, and have an open discussion about it? Ideas tend to get lost when we have brainstorm dumps like the one we did, and this issue now seems to be reduced to 2 open discussions :)

Final tiny note, maybe there should be a list of the task runners that are committed to contribute and support the spec? As for as I know, there is currently http://gruntjs.com/ and http://indigounited.com/automaton/

satazor commented 11 years ago

Bump.

tkellen commented 11 years ago

Just updated the spec to include the task as an instance of an event emitter. We're still going with returned promises to indicate task completion, which will make more sense when the fileReader and fileWriter specs are up.

satazor commented 11 years ago

@tkellen I see that spec props are now attached to the prototype . This is a good decision, since it will allow us to paralelize tasks without any problems.

Still I think you made a mistake. By doing:

var Task = module.exports = require('events').EventEmitter;
Task.prototype.bla = 'name'

We are modifying the EventEmitter protype directly. Shouldn't we use util.inherits or Object.create?

tkellen commented 11 years ago

Just changed that, actually :)

satazor commented 11 years ago

Alright thanks! The spec is looking much more clean now.

tkellen commented 11 years ago

:) Awesome! I'm closing this one for now. We're going to stick with node-task for the name. I'm looking forward to hearing your feedback on the fileReader/Writer additions of the spec.