jokade / angulate2

Scala.js bindings for Angular
MIT License
87 stars 15 forks source link

WIP Generate providers #24

Closed gregghz closed 8 years ago

gregghz commented 8 years ago

I'm looking for feedback on this. This should not be merged as-is.

The main problem is that angular supports a hierarchical DI approach. That is, you only get a singleton in the scope of an injection. Kind of weird.

@Component(
  ...
  providers = @@[HeroService]
)
class Foo(hs: HeroService)

@Component(
  ...
  providers = @@[HeroService]
)
class Bar(hs: HeroService)

I believe each of these will get a unique instance of the HeroService. This is very unlikely to be what we want, especially in a normal application where multiple directives that are children of the root component need common services.

I'm not sure how to overcome this without demanding you to be explicit in the same way the typescript API demands it. I had hoped to remove the providers = ... boiler plate but as I read more about angular2 DI I think this may not be possible without really messing up the semantics.

Making this macro resolve DI scope would likely be more effort than it's worth.

I'd love further input though.

gregghz commented 8 years ago

A solution might be to use tagging

@Component(
  ...
)
class FooComponent(barService: BarService @@ Provided)

To tell the compiler to not add this service to the providers (BarService is already Provided <--- that's the semantics I'm looking for) and instead let angular search up the DI tree.

@@ is what's used by scalaz and other common scala tag implementations. Obviously here that name is already taken.

gregghz commented 8 years ago

I just updated this to implement my idea above. Instead of @@ I use Is so that the semantics (I think) feel natural. "BarService Is Provided" ... which actually means "don't provide BarService in this component (because it's already provided)".

gregghz commented 8 years ago

Maybe a better approach would be the opposite. Tag the dep in the component that you want to own the provider. I think this way it will reduce how often the tag is needed

gregghz commented 8 years ago

ok, I've updated it so that you have to specify that you want a constructor param to be a provider:

@Component(

)
class Foo(a: FooServer ~~ Provider)

is the same as:

@Component(
  providers = @@[FooService]
)
class Foo(a: FooService)

I think the main advantage is that you are able to define local dependencies and provide them in one statement. You don't need to repeat yourself if a provider is also used locally. However you still get to define providers as before in the (probably fairly common) case that you don't have a local dependency.

I'm comfortable merging now if everything looks okay. I'm open to alternatives to ~~ of course.

jokade commented 8 years ago

Sorry for the late response :( I'm not up-to-date regarding the latest API changes in the Angular RCs and I've not checked your code yet, but I'm not quite sure I understand the problem.

My original intention for the Scala.js binding to Angular2 was to keep it as close to the TypeScript API as possible. As you mentioned above, Angular2 uses a hierarchical injection strategy, hence we only need to add providers = ... for root components, don't we?

Maybe we should start a separate separate discussion for the design goals of this binding (I'd prefer a API as close as possible to TypeScript with optional useful enhancements/shortcuts; enforcing a more Scala-like way to do things would be another approach).

gregghz commented 8 years ago

@jokade You're right. In practice (I've spent a lot more time with angular since I submitted this PR) what I've proposed here isn't particularly useful. Most of the time it would only be useful if your root component was both defining a dependency as a provider AND using it in functionality of the component itself. For example:

@Componenet(
  ...
)
class AppComponent(userService: UserServer ~~ Provider) extends OnInit {
  var user: js.UndefOr[User] = js.undefined

  def ngOnInit(): Unit = {
    user = userService.getUser()
  }
}

In this case since AppComponent uses UserService it might make sense to add it as a dependency and provider in one line (you can omit the providers = ... statement when using ~~). I think for most deps this isn't the case though. This PR ends up adding a fair amount of complexity for likely minimal practical gains.

It seems that much more common is to have the root component list everything that should be transitively provided to all other components and let the children components actually depend on and use the thing.

As far as I'm concerned this PR is kind of moot and might be a useful add on later. It's too early in the life of angulate2 to add these kinds of things now though I think.

FWIW, I have been using a (fairly heavily) modified angulate2 and the latest release of angular2 (rc.4 I believe, though I had to patch it slightly as well) and there isn't a ton that has to change internally to angulate2. I've mostly added missing pieces. Routing is pretty different, @RouteConfig doesn't work with the non-deprecated router module but it's easy enough to get routing working almost identically to the TypeScript API. I'll likely have some PRs in the next week or two. Right now I'm rushing to get this all together for a talk I'm giving at OpenWest tomorrow so it's pretty sloppy right now. I think my changes to angular2 are either on master already (I can't test, master seems fairly broken) or would merge in fairly easily.

jokade commented 8 years ago

@gregghz sorry, I overlooked your last comment; ok, you just want to be able to specify the providers inline in the constructor instead of having to define them in the annotation. Ok, but if you have more than one provider, you actually have to type more than less, and we still have to type out the @Component() annotation. Anyway, I'm open to all suggestions that make coding Angular2 in Scala.js more efficient/natural.

However, I'd prefer to have clean and complete implementation of the Angular2 API that is close to the Typescript API at first, and then add Scala-specific enhancements from there on. Would you aggree on that approach?

jokade commented 8 years ago

@gregghz I've started a discussion on some basic design goals for this project. I'd really value your input on this (and any other topic you can think of).

BTW good luck with your talk tomorrow -- I'd be interested in the slides / recording.