empty group()s should not complete automatically.

[broofa]

If the steps for a group are defined asynchronous to the this.group() call (i.e. subsequent to the check() call scheduled by next.group), localCallback() ends up being called prematurely since no functions have been added to the group yet.

The specific case where I ran into this was when trying to do the following, where the group() call appears in the callback to an intermediate, async, function:

step(
  function () {
    var group = this.group();

    LIST_OF_STATES.forEach(function(state) {
      // intermediate async action
      request('url_get_counties_by_state', function(err, countiesInfo) {
        countiesInfo.forEach(function(county) {
          save_to_db(county, group()); // group here
        }
      });
    });
  },
  function (err, counties) {
    // print out list of all counties in all states
  }
);

(Note: This is slightly problematic in the case where this.group() is called in anticipation of some work being done, but where it's later discovered that there's no actual work needed. However that seems like even more of an edge case then the above, where work is being scheduled asynchronously.)

[seanjsong]

OK. Let me explain further. If your problem was like this:

step(
  function () {
    var group = this.group();

    LIST_OF_STATES.forEach(function(state) {
      var callback = group(); // in each loop, call group() synchronously...
      request('url_get_counties_by_state', function(err, countiesInfo) {
        save_to_db(countiesInfo, callback); // ...then pass the result into an async callback
        }
      });
    });
  },
  function (err, countiesInfos) {
    // print out list of all counties in all states
  }
);

Only in this way can you ensure the entries in parameter countiesInfos are in the same order as LIST_OF_STATES.

But actually your problem is more complicated than this:

LIST_OF_STATES.forEach(function(state) {
  // intermediate async action
  request('url_get_counties_by_state', function(err, countiesInfo) {
    countiesInfo.forEach(function(county) {
      save_to_db(county, group()); // group here
    }
  });
});

In an async callback, iterate through a sub list and group them. In this case I suggest you extract the inner code to another function, wrapped by another step().

[broofa]

Only in this way can you ensure the entries in parameter countiesInfos are in the same order as LIST_OF_STATES

As I said, this isn't relevant to the problem. I don't expect the order of countiesInfo to have any correlation to the order of LISTOFSTATES for the reasons you've mentioned - that's not important to me. What's important is that the last step, function (err, countiesInfos) {, should only be called once all save_to_db operations have completed.

I suggest you extract the inner code to another function, wrapped by another step().

Can you provide some example code for this? I haven't the foggiest idea how to do what you're describing and, frankly, I'm a little skeptical that such an approach will actually solve the problem. Unless you're calling step() synchronous to the this.group() call, you won't have an opportunity for that new step() code to execute before the group is prematurely completed.

[seanjsong]

Yes, maybe the order is not relevant to your problem. But it is the concern of the author. And other codes may rely on the order correspondence.

Haven't the foggiest idea? OK, I give you the full code. Hope this can solve your problem:

function saveCountiesInfo(countiesInfo, callback) {
  step(
    function () {
      var group = this.group();
      countiesInfo.forEach(function(county) {
        var countyCallback = group();
        save_to_db(county, countyCallback);
      });
    },
    callback
  );
}

step(
  function () {
    var group = this.group();
    LIST_OF_STATES.forEach(function(state) {
      var countiesInfoCallback = group();
      request('url_get_counties_by_state', function(err, countiesInfo) {
        saveCountiesInfo(countiesInfo, countiesInfoCallback);
      });
    });
  },
  function (err, counties) {
    // print out list of all counties in all states
  }
);

[broofa]

Ahhh, I see! The missing piece of the puzzle was the use of group() outside the scope of the request(...) call to create the request() callback synchronously, which makes sense now that I see it, of course. Thanks!

One possibly undesirable side-effect of this is that the resulting data structure is an array of arrays, rather than a simple flat array of counties. But that's not a big deal.

Yes, maybe the order is not relevant to your problem. But it is the concern of the author. And other codes may rely on the order correspondence.

There's nothing in my pull request that impacts how Step orders the results of grouped, async callbacks, is there? The order in which group() is called is still the order in which you get the results.

[seanjsong]

The reasoning is like this:

There really is nothing in your pull request that impacts how Step orders the results, but you try to allow calling group() from async callbacks. That should not be allowed, otherwise group() could be called in any order, hence breaking the order correspondence.

[broofa]

The promise that Step makes is that results will be returned in the order in which group() is called. Period. This diff does not break that promise.

If a developer chooses to write code that yields non-deterministic ordering of their group() calls, that is their choice. There are times when order isn't important - e.g. a recursive file removal method. In these cases, allowing a simpler grouping structure is a tangible benefit, as opposed to having to refactor code into 2 or more separate Steps phases (and subsequently figure out how flatten whatever results are returned, btw). It's a tradeoff well worth making, imho.

Or to put it another way, I hate it when a framework forces me to obfuscate my code unnecessarily.

[seanjsong]

OK. Even if you insist understanding Step the way you like, your fix doesn't really fix it. What if group() appears several times in a step, some of them are called back synchronously while others are called back asynchronously? Then, when it comes to if (counter > 0 && pending === 0), the condition values to true because some group() have been called back synchronously. So we go on to the next step, and you still lose those group() that will be called back asynchronously.

[broofa]

That's no worse than how things work currently.

[broofa]

crap, I've already run into a case where a group is unexpectedly empty and, thus, fails to complete. :p

justgoaheadandignoreme