Feature request - autoInjectReturnResults

[darrin]

First - thanks for async - it's just awesome! I think I love you guys.

What's more - I'm loving the autoInject function. I have some generic code where I build the set of tasks dynamically - it all works swimmingly with autoInject. However - the code doesn't need a single result or a small subset. Nope, I need the full set of task results - ideally in a map just like auto would return.

I'm currently forking async (darrin/async) to accomplish this. I've created a new function called autoInjectReturnResults that does what I want. The only difference between it and autoInject is that I pass the callback straight-through to auto after the forOwn block here with this:

    auto(newTasks, callback);

Unless I was under the influence I think an early implementation of autoInject as working exactly this way. I'm certain it was changed because this is a bit inconsistent and maybe non-intuitive unless you really know you want this behavior.

Anyone have thoughts on this? Am I the only one that needs to know the full set of task results without knowing apriori what the task names will be?

Thanks in advance,
-Darrin

[megawac]

I'm not sure how I feel about this function depending on the names of parameters. Seems quite sketchy to me. I think I'm 👎 including this in core. Rather than forking, I would recommend you create a library exporting this function so you could do

import async from 'async'
import autoInject from 'async.autoInjectReturnResults'

/cc @aearly

[darrin]

Ha! I'm not sure how or why parameter names would change unless you mean the name of the function itself which I can only agree stinks.

I am curious to know how I could easily just create a library. I'm likely missing something but I tried to do this by defining my new 'sketchy' function which involved copy/paste from async then aforementioned modification to replace the call to auto as described above. Trouble was that forOwn block in autoInject more/less ended up forcing me to pull in much of the async library and it's army of utility functions. At that point - with a deep sigh - I gave up and forked. Idea on how I could get around that or am I missing something more fundamental?

[megawac]

Ha! I'm not sure how or why parameter names would change unless you mean the name of the function itself which I can only agree stinks.

Minification can change parameter names, as you mentioned in the comments

Idea on how I could get around that or am I missing something more fundamental?

Unless I'm misunderstanding, you can import the same lodash methods and async.auto in a third party library and just export your file as the main file.

import auto from 'async/auto';
import forOwn from 'lodash/forOwn';
import arrayMap from 'lodash/_arrayMap';
import clone from 'lodash/_copyArray';
import isArray from 'lodash/isArray';

export default function autoInject(tasks, callback) {
   ...
}

[jdalton]

Yep, relying on param names is no good.

[darrin]

@jdalton @megawac - Yeah I realize that we could have a religious war about use of param names by autoInject (and my suggested variant of same). Since autoInject is already part of the latest version of async I was hoping to steer clear of this debate. I just want to get the full set of results for what is otherwise a mechanism that fits my use case perfectly.

@megawac thanks for the suggestion I'll take a look! The hurdle will be that our nodejs project doesn't currently use ES6. We've been thinking of moving in that direction for a variety of reasons. I don't think this will push us over the cusp but it'll continue to build the case for it.

[megawac]

@darrin you can easily replace ES6 with commonJS

var auto = require('async/auto');
var forOwn = require('lodash/forOwn')
var arrayMap = require('lodash/_arrayMap');
....

module.exports function autoInject(tasks, callback) {
   ...
}

[jdalton]

Yeah I realize that we could have a religious war

Nothing religious about it. It breaks when minified so it's a non-starter.
Parsing a function's args is clunky and error prone as well.

[darrin]

@jdalton - The writer of autoInject (@aearly - NOT ME) already anticipated your argument which is why autoInject supports AngularJS array style invocation which deals with your minification concern. I understand and accept the limitations of autoInject (except that I'd like to get the full set of results out!). In my opinion this is an awesome bit of logic that significantly lowers maintenance costs and makes easier to understand what would otherwise be a fairly complex region of code. WIN. WIN.

@megawac thanks - I'll give that a shot. Definitely agree I'd like to get away from the fork.

[aearly]

The original original author (@steverobb) of autoInject created it that way. I had ported it, but left out how it unpacked the results into the callback. See #1099 and #1100 for more context, that's where the feature was added back in. I would be more inclined to just return a results object, just like plain auto. It's not too late to change it back.

[darrin]

Well - I do understand why you did it. There is a symmetry to the current
approach.

I'm not the most objective to argue the case for changing it back BUT of
course I think you should. ;). Returning all the results is simpler, allows
for greater flexibility and is (slightly) more efficient. In my situation
I'm passing both the functions to process and the post processing into a
larger function. It's not possible with the current solution.
On Apr 22, 2016 5:39 PM, "Alex Early" notifications@github.com wrote:

The original original author (@steverobb https://github.com/steverobb)
of autoInject created it that way. I had ported it, but left out how it
unpacked the results into the callback. See #1099
#1099 and #1100
#1100 for more context, that's
where the feature was added back in. I would be more inclined to just
return a results object, just like plain auto. It's not too late to
change it back.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1122 (comment)

[steverobb]

As the author of the original feature, I'm not the most objective to argue the case for leaving it as it is BUT of course I think you should. ;)

One of the aims of autoInject was to give a very clear picture of dependencies throughout the construction of the task graph. This is a readability and maintenance win.

I do see your need though - even if you don't do the sketchy thing of relying on parameter names, I can see it being useful to have packed results for ease of passing around, so there's no point in autoInject unpacking them only for you to repack them.

However, auto already gives you that, so you could just use that instead of autoInject. I guess that's not desirable either?

I originally tried to fold the packing/unpacking behaviour into auto. I considered reserving a 'results' parameter name (and 'callback') to make that work, but that seemed terrible. In the end, it just seemed a lot easier to create a new function rather than overload auto further.

tl;dr: I think autoInject should stay as it is. I think a new function like autoInjectReturnResults is reasonable to have.

[darrin]

@steverobb Thanks so much for contributing your thinking here and for writing autoInject! I was thrilled and surprised when I found autoInject. It makes something that was going to be really messy, hard to test and hard to maintain, imminently manageable!

Yes, the readability and maintenance win is what I'm going for here. I think it's worth it to try to convince you that the original way you wrote it is worth sticking with. An (admittedly contrived) example would help me explain why I think what we're calling autoInjectWithReturnResults is the best solution if those are our objectives:

Let's assume we want to calculate some series of logical statements. Further assume we want to be able to say WHY any given statement was true or false so we really want all the calculated dependencies back. Assume there are many dependencies for some functions. Lastly the statements are likely to change over time as we add new logic - so we know we'll have to manage changes to the code.

Below I'll show 3 examples - one for each approach. I'm highlighting a more complex function definition ('g' in this case) and leaving the others out. Also I'm assuming that we leverage parameter names for mapping - (I don't care or need minification in my use-case) - so I'm using the abbreviated syntax:

With auto

Here

async.auto({
    g : ['a', 'b', 'c', 'd', 'e', 'f', function (deps, callback) {
        return callback(undefined, 
                            (deps.a && deps.b && deps.c) || (deps.d && deps.e || deps.f));
    }],
....
    function(err, results) {
         // show that g is true or false and why.
    }

Pros:

1. Automatically resolves dependencies and executes in order.
2. Fails immediately if it cannot resolve listed dependency.
3. Nicely bundled results at the end!

Cons:

1. I need to manually maintain the relationship between the string parameter-dependencies and the code that pulls the variables out of deps.
2. Does NOT fail if we fail to list a dependency! Failure to maintain the proper relationship between manually listed dependencies and consumed deps may not be detected at runtime since the map will return undefined for an unmapped result rather than throw an error. We might fix this by checking the keys in deps when we start each function but then we need to write even more code which we would have to maintain AND keep consistent.
3. Similarly - because of the constructs being used, especially the use of strings, the IDE cannot tell if a parameter is missing bc it really doesn't have a clue what should and should not be in the deps map.
4. Nice bundle of results is returned at the end so we can support generic inputs with a generic result handling function at the end.

With autoInject

async.autoInject({
    g : function (a, b, c, d, e, f, callback) {
    return callback(undefined, (a && b && c) || (d && e || f));
    },
....
    function(err, a, b, c, d, e, f, g) {
         // convert all results to a map
         // show that g is true or false and why.
    }

Pros:

1. Automatically resolves dependencies and executes in order.
2. Fails immediately if it cannot resolve listed dependency.
3. No need to manually maintain list of dependencies (unless you want to minify).
4. Function signatures are directly mapped to variables used in the function making syntax clear and explicit!
5. Because of 4 - the IDE can detect when a variable is used BUT not defined in the signature!

Cons:

1. We need to be explicit about the results at the end - if we want it all - we have to remember to include it all...
2. Related to the first reason - we cannot easily support a dynamic set of functions passed to autoInject unless we also dynamically define a function to process the results at the end.

With autoInjectReturnResults

async.autoInjectReturnResults([
    g : function (a, b, c, d, e, f, callback) {
    return callback(undefined, (a && b && c) || (d && e || f));
    },
....
    function(err, results) {
         // show that g is true or false and why.
    }

Pros:

1. Automatically resolves dependencies and executes in order.
2. Fails immediately if it cannot resolve listed dependency.
3. No need to manually maintain list of dependencies (unless you want to minify).
4. Function signatures are directly mapped to variables used in the function making syntax clear and explicit!
5. Because of 4 - the IDE can detect when a variable is used BUT not defined in the signature!
6. Nice bundle of results is returned at the end so we can support generic inputs with a generic result handling function at the end.

Cons:

1. Maybe people will expect that last function to also be autoInjected - so they'll be confused by a single return of mapped results?
2. Obnoxious name.

My use case is actually complicated a bit more because the set of functional dependencies to run is dynamic - so forcing me to be explicit with that last function definition makes life... rather difficult.

Still - I don't think we need to go that far - the examples above I think pretty clearly help show what the benefits are even assuming dynamically changing functions aren't your use case... To me, the last option appears the best choice if what we're trying to do is make other's code more terse, easier to understand, flexible and maintainable. It's also the shorter and more efficient implementation. autoInject as written also makes sense but to me we loose out by forcing the autoInject at the end.

Whew... sorry for the book. That's the best I'm willing to do to make a case for this. If you don't end up providing a function that gives me the full result bundle at the end I will of course understand and still feel that bit of code is genius! BUT I'll definitely stick with my fork or better get it working as @megawac suggested above.

Thanks again! Awesome work!
-Darrin

[steverobb]

You really didn't need to write a book - as I already said, I agree with your argument for your use case. I just don't think your and my use cases are mutually exclusive.

And I don't personally agree that having to list your result dependencies explicitly is objectively a negative - maybe it is in your case, but it certainly isn't in mine. A results object makes much of my code less terse and less readable. Explicitness of dependencies is part of the reason why I wrote autoInject in the first place: for clarity of reading dependencies so that obsolete (and thus removable) subgraphs can be quickly inferred during maintenance.

Ultimately, it's not my library, so @aearly can choose to go with his preferred implementation (and he has already suggested a preference for composite results). If he chooses to go that way instead of yet another auto variant, then I could change my graphs to the following to make them slightly worse but still explicit:

// Before - current implementation
async.autoInject({
    a: function(cb)       { do_task_a(cb); },
    b: function(cb)       { do_task_b(cb); },
    c: function(a, b, cb) { do_task_c(a, b, cb); },
}, function(err, c) {
    if (err)
        handle_error(err)
    else
        use(c);
});

// After - composite results object
async.autoInject({
    a:        function(cb)       { do_task_a(cb); },
    b:        function(cb)       { do_task_b(cb); },
    c:        function(a, b, cb) { do_task_c(a, b, cb); },
    assemble: function(c, cb)    { use(c); cb(); } // need to remember to call cb() here to terminate the graph!
}, function(err, /*results*/) {
    if (err)
        handle_error(err);

    // don't use results
});

[steverobb]

I've been experimenting with my workaround above and I feel that it makes some of my graphs slightly less readable and others slightly more readable. A net lack of change in overall readability.

As I still get my explicit dependencies and the tide seems to be in favour of composite results, I'm fine with reverting this back, so I have: #1126

Hopefully everyone now wins. :-)

People might still expect the final callback to be injected, as per @darrin's first 'con' for autoInjectReturnResults above, but perhaps that's just a documentation issue.

[aearly]

Yeah, after giving this more thought, I like returning a result object, rather than injecting individual results. By default, the final callback omits data -- it would be better if autoInject returns everything it has so it's more flexible in how users can extract desired results.

The minification issue still exists, and though you can work around it by passing an array with the param names as strings, it's still odd to not have a plain callback function as the last arg.

[aearly]

Closed by #1126