[feat] Composition through properties decorator

[cojack]

I'm submitting a...

[ ] Regression 
[ ] Bug report
[x] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

Right now if you're extending classes and want to use some "objects" in the "parent" class, you have to pass them individual from the child to parent. DI doesn't handle this behaviour.

Expected behavior

I would like to see something different than DI through constructor, so Composition as property decorator:

export class Foo {
    @Composition();
    protected bar: Bar;

    // logic
}

export class FooChild extends Foo {
    constructor(private ownBar: Bar) {
        super();
        console.assert(this.ownBar === this.bar, 'How does it feel?');
    }
}

Minimal reproduction of the problem with instructions

None.

What is the motivation / use case for changing the behavior?

Even better/fancy/sexy dependency injection. This will prevent us to pass deps through constructor, code will looks much more cleaner, less effort.

Decorator Composition might also takes a some params, like Model from which we would like to get type.

What do you think guys?

Environment

Nest version: 5.1.0


For Tooling issues:
- Node version: v8.11.2  
- Platform: Linux  

Others:

[BrunnerLivio]

This proposal has already been discussed on Stackoverflow with Angular.

Günter Zöchbauer on Stackoverflow:

It hides dependencies which makes code harder to read. It violates expectations of one familiar with how Angular2 DI works. It breaks offline compilation that generates static code to replace declarative and imperative DI to improve performance and reduce code size.

I think it is cute proposal, but as Günter mentioned the code gets harder to read. Sure if you wrote the code on your own it is understandble. But imagine you’re a new developer to a project and have to edit super class which gets inherited by unknown dependencies. You do not know that some properties of this class get injected by an inherited class. I think the Angular team did not include this on purpose. I am not an architecture specialist but I think the guys from the Angular team are. So I trust them, but open for your ideas :)

[cojack]

It hides dependencies which makes code harder to read.

Im sorry, but I don't agree with the point that the code is harder to read. It doesn't matter, why should? How? When? Where?

This is simple case, when you see some decorator, first you're looking from which package this decorator comes from, second you will look for a documentation, what this decorator do, just it. Harder to read it the flow of dependencies, do you remember callback hell? Is the same, pass argument to call it latter, why? There is no reason for that.

It violates expectations of one familiar with how Angular2 DI works. It breaks offline compilation that generates static code to replace declarative and imperative DI to improve performance and reduce code size.

not related at all

Here is example of property decorator: https://github.com/wookieb/alpha-dic/blob/master/docs/decorators.md from another DI

[BrunnerLivio]

I see your arguments. Maybe I did prematurely raise some arguments, because I researched more on property injection (which is in my opinion a terrible thing) but thinking more about it I realized your proposed inheritance-only-property-injection would solve some issues that property injection has.

I still think it is bad practice because it contradicts the object oriented flow in my opinion. The super class which has the Composition property would only be correctly instanceable with Nests dependency injection (unless you verify everytime the property gets called it is not null). You can not naturally set a value to the protected property, therefor makes it null by default.

In the end it feels like a personal preference. I am against it and I would not use this feature, even if it gets accepted.

[cojack]

@BrunnerLivio even more, Spring have similar construction, called Autowire and I thing this is quite awesome and useful. Check it here https://www.baeldung.com/spring-autowire and I think guys who are writing Spring have even more experience. So I will follow them, copy best solution and inject them into Nest, but not like blind person.

You can not naturally set a value to the protected property, therefor makes it null by default.

Protected/private it doesn't matter after transpilation, they are all public, unless you convert it to ES5 then they are created in the way like this:

function Foo () {
 var myPrivate = 2; // only if initialized 
}

And this might be a problem. But, there is no reason for transpile code to ES5.

In the end it feels like a personal preference. I am against it and I would not use this feature, even if it gets accepted.

I see, but don't forget that nobody will force you to use it ;)

[kamilmysliwiec]

Personally, I really don't like this feature. Obviously, in some cases, it may reduce some boilerplate code, continuously passing arguments up to the class hierarchy might feel annoying. However, it is still pretty straightforward that we need to delegate parameters in a traditional way, this is something language-specific and we are used to doing it in this way.

Nevertheless, I'm leaving this thread opened, would love to hear more voice from the community. 🙂

[danosier]

I have to agree with the original poster here. I am currently running into this specific scenario in something I am working on. Having to use only constructors for DI is causing me to repeat a lot of the same code. Its possible and it works, but its not very pretty. I understand the reasons for not wanting to do it, or even recommend it, but it would be nice to have the option. My 2 cents

[marcus-sa]

Inversify does this optionally, so yet another issue that could be solved by https://github.com/nestjs/nest/pull/1127

[kamilmysliwiec]

I also encountered a case when this functionality could be useful. Still can't decide in terms of the design perspective, but it's a truth that sometimes it might be helpful.

[BrunnerLivio]

I kinda loosen my perspective on this topic too. I still see it as bad practice, but if someone really wants to use it, we should not restrict it. However I think the docs should explicitly say this is bad practice and you should consider other patterns (container injection).

[danosier]

Admittedly I don't know enough about typescript and this framework, but could you make it so one could use optional parameters in the constructor of the super class? If the child class provides the property, use that, but if not, use a default injected one from the super class?

[kamilmysliwiec]

see #1172

[cojack]

@kamilmysliwiec did I told you that I love you? <3

[kamilmysliwiec]

lol ❤️

[PatrickGeyer]

@kamilmysliwiec does this mean we can do this:

`export class BaseClass {

    @Inject(forwardRef(() => RequiredService)) private requiredService: RequiredService;

    constructor() {
    }
}`

I get this error:

[ts] Unable to resolve signature of property decorator when called as an expression. [1240]

Which I'm guessing has nothing to do with NestJS, but is a Typescript error.

[kamilmysliwiec]

@PatrickGeyer please, update your nest packages :)

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.