Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

autoInject - dependency injection for auto. #608

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 31, 2014

Dependency injection support for auto, for better readability of task graphs.

async.autoInject({
    template:        function(cb)                         { fs.readFile(templateFilename, cb); },
    serverData:      function(cb)                         { http.get(dataUrl, cb); },
    templateData:    function(cb, serverData)             { formatDataForTemplating(serverData, cb); },
    appliedTemplate: function(cb, template, templateData) { mustache.to_html(template.toString(), templateData, cb); }
}, function(err, appliedTemplate) {
    if (err) return handleError(err);
    Serve(appliedTemplate);
});

@aearly
Copy link
Collaborator

aearly commented Sep 9, 2014

Pretty cool, definitely would clean up uses of async.auto in a lot of cases. I'm a bit apprehensive that it relies on Function.toString() -- makes it harder to plug-and-play existing functions and precludes the use of variadic functions, but it seems autoInject's use case would be for inline function expressions where you declare exactly what you expect. Also -- any reason why the callback is the first arg, rather than the last, as per node.js conventions?

@ghost
Copy link
Author

ghost commented Sep 11, 2014

autoInject inherits most of the issues you mention from auto.

The first readData example in the auto docs mention the need to supply a wrapper function because the function signature not being strictly compatible with fs.readFile. I can't speak for everyone, but I've never seen auto used without wrapper functions. I'd always use them myself, for readability.

I originally had the callback at the end of the function, but I changed it for two reasons:

  • It felt a little strange that the 'variadic' parts of the function signature were the initial parameters, rather than at the end. You'd always need the callback as the last parameter, otherwise the implementation wouldn't know if your argument was intended to be the callback or the name of a dependency (unless you reserved a name for the callback, which I didn't like the idea of), so the usual trick of omitting trailing parameters of unneeded arguments wouldn't work.
  • auto has the callback at the front. When migrating some auto usage to autoInject, it made sense to replace function(cb, results) { /* use results.a, results.b */ } with function(cb, a, b) { /* use a, b */ }.

It made the implementation a little cleaner too, though I don't really regard that as a motivating factor.

A couple of my own points about the implementation:

  • Task names must be legal JavaScript identifiers so, unlike auto, they can't (e.g.) start with a number or contain symbols. I have not found this to be an issue in practice.
  • It might be necessary to consider other names for the function if there is concern that this is some kind of hybrid of auto and inject. It has a lot to do with auto and nothing to do with inject. But dependency injection is what it does, so the current name is relevant.

@aearly
Copy link
Collaborator

aearly commented Jan 2, 2016

BTW, I'm liking this more as I think about it. It'll just have the same minification caveats as Angular. It'll be really handy in server-side code.

@steverobb
Copy link

Yeah, server-side and Node scripts were mostly what I was thinking about when I wrote this feature. And auto will still exist for code that needs to be minification-aware.

I notice in #957 that you are planning to always have the callback last. Should I make that change here or were you planning to make it yourself after merging?

@aearly
Copy link
Collaborator

aearly commented Jan 5, 2016

Yeah, I'd prefer to always have the callback be the last argument. No sense in doing it one way if we're just going to turn around and change it soon after.

aearly added a commit that referenced this pull request Mar 9, 2016
@aearly aearly mentioned this pull request Mar 9, 2016
@aearly
Copy link
Collaborator

aearly commented Mar 9, 2016

Closing in favor of #1055

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants