aurelia / dependency-injection

A lightweight, extensible dependency injection container for JavaScript.
MIT License
160 stars 66 forks source link

Proposal for easier way to inject via DI into a view-model #156

Closed Vheissu closed 6 years ago

Vheissu commented 6 years ago

I find Aurelia to be super efficient in terms of development, but there is one thing that takes up a few seconds of my time is dependency injection by needing to specify a constructor and use passed in injectables as params, then alias them to the current class.

To inject the Event Aggregator, you do this at present (I use static inject, but many use the inject decorator):

import { EventAggregator } from 'aurelia-event-aggregator';

export class MyViewModel {
    static inject = [EventAggregator];

    constructor(ea) {
        this.ea = ea;
     }
}

I am proposing a new decorator which does what the above code does, but looks like this:

@injectable(EventAggregator)
export class MyViewModel {

}

Under the hood, things would work the same. The decorator would simply be specifying the injected instances the same way @inject does and then would be creating new properties on the class itself.

The proposed decorator would have two arguments:

This is an example of an alias being provided (overriding default convention):

@injectable(EventAggregator, 'ea')
export class MyViewModel {

}

The above example would inject the Event Aggregator into the class as this.ea.

Why?

We already have flexible injection options and if you're a TypeScript user, you can @autoinject and can hoist params on the constructor to the class, but not everyone uses TypeScript and it still doesn't solve the problem of needing to define a constructor, in many cases just to pass DI instances into the class.

This is a case of me being lazy and wanting to clean up my view-models, because in many cases I never do anything inside of a constructor other than pass in DI instances.

Caveats

From what I can discern, there would be no issues with something like this. Although, I would be surprised if this hasn't been thought of before and perhaps there are technical challenges with doing such a thing preventing a decorator like this ever being feasible. Happy to hear from others smarter than myself to chime in whether or not what I am proposing is crazy or possible.

bigopon commented 6 years ago

It could be a good chance to enable @autoinject for Js. But i think we should not introduce new decorator Consider:

const isMultiple = true;
@inject({
  ea: EventAggregator,
  el: Element
}, IsMultiple)
export class MyViewModel {

}
Vheissu commented 6 years ago

Leveraging inject is a great idea.

stsje commented 6 years ago

What you are describing is property injection. I don't like the idea of implementing a new injectable decorator, but maybe instead we could do property injection like we know it from autofac, guice etc.?

TypeScript will be easy enough, since it already knows the type of the object

export class MyViewModel {
  @inject eventAggregator : EventAggregator;
  @inject({ resolver: Lazy }) httpClient : HttpClient;
}

JavaScript you have to tell the decorator the type

export class MyViewModel {
  @inject(EventAggregator) eventAggregator;
  @inject({ type: HttpClient, resolver: Lazy }) httpClient;
}
stalniy commented 6 years ago

The main issue with this is that it introduce property injection. In most cases constructor injection is preferred just because it will protect object from being constructed in semi-working state. For example, lets imagine that each Dog must have owner:

class Dog {
  constructor(owner) {
    this.owner = owner
  }
}

Now, you are forced to provide owner object in Dog constructor, otherwise you can't create a Dog. If we move owner to property injection:

@inject({ owner: Owner })
class Dog {
}

Then it will be possible to create a Dog without owner, what may result in potential logical issues later.

So, I'd suggest to handle this with babel/typescript plugin which converts this custom decorator into constructor. However it may complicate things a bit because if user provide its own constructor you will need to think how to merge both

zewa666 commented 6 years ago

Well this is kinda where TypeScript gets handy with the constructor assignment feature:

import { autoinject } from 'aurelia-framework';
import { EventAggregator } from 'aurelia-event-aggregator';

@autoinject()
export class MyViewModel {
    constructor(private ea: EventAggregator) {}

    // automatically binds ea to this.ea
}

Also as @stalniy mentioned this would turn into property injection vs constructor injection, which could in turn hurt the testability of your class.

I personally think this small fix isn't worth it because of two reasons:

A: Its actually not that much that you'd save and TypeScript already allows a short hand syntax. B: Don't go with a custom decorator as this creates a higher coupling between your code and the framework. Constructor injection is a high-level pattern which is not specific to a framework, the inject decorator is, which is also a reason why some people prefer static inject vs the decorator approach.

fkleuver commented 6 years ago

Saving only a couple of statements per class is quite a big saving on a class that only consists of a couple of statements to begin with.

And it's not just having less code to write, it's having less code to rewrite when you refactor. That's a huge timesink in frontend development where rapid prototyping and quick POCs play a dominant role.

"you can't have a dog without an owner" is an argument for idiomatic DI in server-side applications. In the frontend you almost never need all deps right after the constructor, you need them before the next lifecycle hook. And if you do need something during construction, just inject that one thing in the constructor!

As for coupling with the framework.. property injection via decorators doesn't seem too framework-specific to me. It exists in practically every serverside DI library (where decorators are attributes) as well as the only clientside DI library I was able to find

This:

export class MyViewModel {
  @inject ea: EventAggregator;
  @inject tq: TaskQueue;
}

Is cleaner and easier to refactor than this:

@autoinject()
export class MyViewModel {
  constructor(ea: EventAggregator, tq: TaskQueue) {}
}

It's just a single line to select and add/remove/move. It's the small things.

You could have the constructor arguments on single lines but keep in mind formatting tools kind of ruin the party here unless you manage to configure them to always put constructor arguments on their own lines, but not method arguments.

zewa666 commented 6 years ago

Its not about the prop injection itself that is framework dependent but about the actual decorator. Remove autoinject and you can use the whole VM in Angular or Vue or whatever. With the property injector you have to translate that first. Also about the Dog and owner topic, constructor injection forces best practices. You save a line If you skip it but might get heavier times resolving issues or have harder times creating mocks during testing.

fkleuver commented 6 years ago

Remove autoinject and you can use the whole VM in Angular or Vue or whatever.

I don't understand how you can say this. Of course such a migration will never be as simple as removing autoinject, nor will property injectors ever be a significant part of the work needed.

Look, I know the best practices and I wholeheartedly agree with you on principle. However as a senior developer I am fully confident in my ability to judge when I can or cannot "break a rule".

People who know the idioms won't use it when not appropriate, and people who don't know the idioms will write bad code anyway.

My point is that I'd want this specifically for the "demo / rapid prototyping / POC" use case. Don't even have unit tests yet. 2 things consistently cost me more time and energy than anything else when prototyping. DI is one of those 2 things, and the other one was router configuration which I solved with a plugin (which is where that code belongs).

Property injection seems like a fairly normal and common feature for a DI framework, but if my use case is so rare then I guess a plugin it is.

Honestly, I'd understand (and agree with) arguments regarding complexity or performance degradation. None of the current counter-arguments however really hold that well for me. Aurelia is not meant to be a teaching vessel. Just my 2c

EisenbergEffect commented 6 years ago

Closing this. There's a community plugin that hooks into DI and adds property injection via the inject decorator. https://github.com/devaur/aurelia-property-injection

fkleuver commented 6 years ago

Had no idea, thanks!