ethul / purescript-angular

AngularJS 1.2 bindings for PureScript (currently in the experimental stage)
MIT License
23 stars 3 forks source link

DI: Automatically infer and annotate dependencies for injection #14

Closed dylex closed 9 years ago

dylex commented 9 years ago

What do you think of this? It allows you to automatically add dependency injection annotations for existing functions, eliminating the need for the ngInject stub functions. You can see how it works by the updated example.

I'm new to purescript so this is really a first pass/proof-of-concept but it seems to work as is.

ethul commented 9 years ago

Thanks for the PR. I will dig into this soon!

ethul commented 9 years ago

Overall, I think removing the need for the ngInject stub functions would be great! I'd like to have it so that users of the library can go for any annotation method they want. For example, some might want to keep the stub functions and run the resulting javascript through ng-annotate as part of their build process. While others might want something like you are proposing.

Regarding your proposal, is there good way to provide this as a dependency to controllers and providers? Or maybe there is a better way to handle obtaining access to this. What do you think? Also, I didn't try this out, but do you know if annotate will work for in-lined controllers in directives? When you want to annotate something like:

module.directive('test', function(){
  return {
    controller: function($http, $router, /*etc*/){
      ...
    }
  };
});

I suppose you'd have to separate out the controller function and annotate it. Similarly if you wanted to annotate something like:

$httpProvider.interceptors.push(function($router){})
dylex commented 9 years ago

Is there a standard way to access this in purescript? I changed it to preserve this in the final call, but that's probably not so useful. Could easily make a newtype This a and fake "this" dependency that passes it as an argument, if you think that's the right way.

Obviously this doesn't prevent people from continuing to use stub functions, it just provides an alternative. In your first example you should be able to write it all without any imports, as you said:

controller http router = do ...
directive = do
  return {
    controller: annotate controller
  }
main = directive "test" (annotate directive) mod
ethul commented 9 years ago

Looks good. Thanks for making the fixes for the directives.

Regarding this, I don't believe there is a standard way to access it in purescript. I don't know if faking it is the best solution, but I am not sure of a better one at the moment. I think it is something that would be useful. If it's not too much trouble, can you add it in as a fake dependency? Or maybe we can brainstorm on a better approach.

Also, In order to make it clear to users that there are multiple ways of handling annotations, could you perhaps keep one directive (or the controller) using the ngInject stub function? But maybe we should add a very small new example demonstrating the different ways annotations can be handled. What do you think?

dylex commented 9 years ago

This version adds a fake "$this" dependency and handles it manually in annotate. It also adds a bit more type safety distinguishing services (in the injector) and local dependencies. I changed the example only to annotate the controller and leave the directives as-is. I also added this as a dependency of the controller, though it does nothing with it. Another example or maybe adding a service to this one might be good... I'll think about it.

ethul commented 9 years ago

Great, thanks for making these changes.

Do you have an example using the this dependency? I am trying to use it as:

<div ng-controller="TodoCtrl as todoctrl"></div>

However, this fails to instantiate the controller. It looks like the prototype for this is the function g. I don't think this should be the case.

Are you getting the same thing? Maybe I missing something here.

dylex commented 9 years ago

I didn't try anything too fancy, but it seems to work for me. In what way does it fail to instantiate the controller?

It's unfortunate but expected that the prototype is g because of the way angular instantiates controllers. I suppose we could copy the prototype over from the function passed to annotate like so, but I'm not sure there's much point since purescript doesn't provide any way to set prototypes anyway.

ethul commented 9 years ago

Strange. As soon as I add something like <div ng-controller="TodoCtrl as todoctrl"></div>, I get

TypeError: undefined is not a function
    at g

I must have something going on here. I will dig into this more.

ethul commented 9 years ago

Note that if I clone your example from your PR, I get the same error.

dylex commented 9 years ago

Ah, I see the problem. If you were using the controller twice (on different elements), the second function application would re-use the first -- this should fix it. Sorry, still figuring out purescript.

ethul commented 9 years ago

Looks great, thanks for your help and for the new feature!