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

Support asynchronous invoke()'d functions #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

caitp
Copy link

@caitp caitp commented Dec 8, 2013

Similar to ngRoute's resolve property in route configuration, it should be possible to support asynchronous module dependencies in this fashion.

I opened karma-runner/karma#851 on karma a while ago, but it seems like the simplest solution would be adjusting the DI mechanics.

I'll work on a patch for this, but it would be good to hear thoughts about it.

... hmmm I guess it's not easy to do this without changing the API :(

Would it be okay for invoke() to return a promise, so that it could wait for promises to be resolved? (realizing fully well that this would be a pretty breaking change)

...

Alternatively, a similar approach to grunt --- call a module with a context containing a method like async() which would return a means to notify the caller that it was finished... and just block until everything is finished... although that might not necessarily work either. hmmm :(

Injector.invoke accepts optional 3rd parameter, a callback to notify that
the invoked function has completed.

If invoked with a callback, the context of the invoked function is supplied
with method `async()` (as in gruntjs), which returns a method for invoking
the supplied callback with a single value indicating the result of the method.

This could potentially be used to support asynchronous plugins in Karma, with
some work.
@caitp
Copy link
Author

caitp commented Dec 8, 2013

I'd really like to make this happen, because I really want the power to write an asynchronous plugin for karma... I'm not sure if this is necessarily the right place to make that work, but it's a start.

If/when you have time to take a look and see if there's anything else needed to make this work, or ways to work better, I would appreciate it

@caitp
Copy link
Author

caitp commented Dec 8, 2013

Sigh, this doesn't look like it would do it either :( I guess get() needs some work too.

I guess the patch is really just about talking about ways to go about it, because this whole approach really sucks :(

problems:

  • The currentlyResolving stack doesn't work well with asynchronous dependencies (gives the illusion of circular dependencies)
  • It's not desirable to cause necessary API changes
  • How to do this transparently, and without breaking circular dependency detection?

@thom4parisot
Copy link

That would be excellent to achieve private preprocessing in karma for example.

As I'd like not to leak browserify configuration within a user's project.

var browserify = require('browserify');
var b = browserify();

var initTape = function(files){

  return function(done){
    var tapeBuffer = new Buffer();

    b.add('./path/to/tape/index.js');
    b.bundle();

    b.on('data', function(chunk){
      tapeBuffer.concat(chunk);
    });
    b.on('finish', function(){
      files.unshift({ pattern: 'data:text/plain;base64,' + tapeBuffer.toString('base64'), included: true, served: true, watch: false, });
      done();
    });
  };
};

initTape.$inject = ['config.files'];

module.exports = {
  'framework:tape': ['factory', initTape]
};

@vojtajina
Copy link
Owner

@caitp sorry for such a delay, I had notifications for this project turned off and completely missed this. I don't actively maintain this project anymore, in favor of angular/di.js. Even though Karma still uses this DI.

The invoke might return a promise, in which case, you don't have to break the API.

So how about changing the framework instantiation in Karma to wait for all frameworks to finish their init (they can return a promise)?

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

Successfully merging this pull request may close these issues.

3 participants