500px / gunter

Language agnostic task wrapper and loyal servant.
MIT License
9 stars 1 forks source link

sequest is async #8

Closed devonoel closed 9 years ago

devonoel commented 9 years ago

For some reason, sequest's .write behaves asynchronously. It'll throw a bunch of commands at the server, and will immediately continue on with the function without waiting to see how they turn out. shelljs on the other hand is synchronous, which is what I'd prefer here. This causes command events to fire prematurely.

This can be solved by passing .write a callback function where events are emitted instead of just emitting later in the caller function. This is what I've done in the case of end events. The final .write, which closes the connection to the server in a callback function, also returns the callback that emits an end event.

However, in the case of emitting command events, I pass in a param containing the command that was run, which the event subscriber can reference. Because I'm iterating through an array of commands, and referencing the command in question by an index set in a for loop, there is no way (that I can see) to reference the specific command that was run. .write also does not return any useful information which I can pass to the event subscriber instead.

Hopefully, this issue can be solved as part of #7.

devonoel commented 9 years ago

Here's a simplified breakdown of the issue:

function run(task, callback) {
  var commands = task.commands;

  for (var i = 0; i < commands.length; i++) {
    try {
      runner.run(commands[i], function(){
        // commands[i] will return undefined, because the index i is
        // no longer defined by the time the callback is triggered
        emitter.emit('command', commands[i]);
      });
    } catch(err) {
      return callback(err);
    }
  }

  return callback(null, task);
}

I have a for loop where I'm iterating over an array of commands. For each command, a runner asynchronously runs the command, and calls an anonymous callback function that emits an event when the command completes. I'm trying to pass the command that was just run to the emitter, so that the subscriber can do something with it, but because the index i is no longer defined when the command event is emitted, commands[i] returns undefined.

The runner comes from a third party library, so changing its behaviour is undesirable. The runner also does not pass anything to the callback function.

Melraidin commented 9 years ago

Something like this?

function run(task, callback) {
  var commands = task.commands;

  for (var i = 0; i < commands.length; i++) {
    try {
      runner.run(commands[i],
        (function(j) {
          function(){
            // commands[i] will return undefined, because the index i is
            // no longer defined by the time the callback is triggered
            emitter.emit('command', commands[j]);
        }
      })(i));
    } catch(err) {
      return callback(err);
    }
  }

  return callback(null, task);
}
meagar commented 9 years ago

@DevoNoel What Kevin said. You need a function-within-a-function inside your loop which can receive its own i, as all instances of your function currently close over the same i.

As an alternative, drop the for (;;) and use [].forEach, so that each iteration gets its own counting variable for free

meagar commented 9 years ago

function run(task, callback) {
  task.commands.forEach(function (command) {
    try {
      runner.run(command, function() {
        emitter.emit('command', command);
      });
    } catch(err) {
      return callback(err);
    }
  }

  return callback(null, task);
}
devonoel commented 9 years ago

Perfect, thanks for the help guys. The function within a function is a little dense, so I'm gonna use forEach

meagar commented 9 years ago

As an alternate alternative, use CoffeeScript :p

devonoel commented 9 years ago

And run a task runner within my task runner to compile it? Should I make a horribly outdated Inception joke, or I horribly outdated Pimp My Ride joke?