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

Deprecate @Host(), @Inject(), @Optional(), @Self() & @SkipSelf() #50439

Closed
yjaaidi opened this issue May 23, 2023 · 15 comments
Closed

Deprecate @Host(), @Inject(), @Optional(), @Self() & @SkipSelf() #50439

yjaaidi opened this issue May 23, 2023 · 15 comments
Labels
area: core Issues related to the framework runtime
Milestone

Comments

@yjaaidi
Copy link
Contributor

yjaaidi commented May 23, 2023

Which @angular/* package(s) are relevant/related to the feature request?

core

Description

Considering TypeScript 5's support for ES Decorators which do not currently support parameter decorators, we could deprecate all parameter decorators and recommend the usage of inject() instead.

This would allow us to safely disable experimental decorators in the future.

Proposed solution

Deprecate parameter decorators.

Additional Details & Motivations

#50439 (comment)

@jessicajaniuk jessicajaniuk added the area: core Issues related to the framework runtime label May 23, 2023
@ngbot ngbot bot added this to the needsTriage milestone May 23, 2023
@yjaaidi yjaaidi changed the title Deprecate @Host(), @Inject(), @Optional(), @Self() & @SkipSelf() Deprecate @Host(), @Inject(), @Optional(), @Self() & @SkipSelf() May 25, 2023
@amariq
Copy link

amariq commented May 25, 2023

So, instead of this,

class A {
  constructor(@Inject(SOMETHING) private something: Something) {
    // stuff
  }
}

class B extends A {
  constructor(@Inject(SOMETHING_NEW) somethingNew: Something) {
    super(somethingNew);
  }
}

the new code would look like that?

class A {
  constructor(private something: Something | null) {
    this.something ??= inject(SOMETHING);
  }
}

class B extends A {
  constructor(somethingNew: Something | null) {
    super(somethingNew ?? inject(SOMETHING_NEW));
  }
}

It's OK then, except when someone who makes the A class does this,

class A {
   this.something = inject(SOMETHING);
}

then it's not really OK to me. Though that is already possible to do at this point, but deprecating the decorators will likely force many to use the shorter option despite its drawbacks regarding extension and reuse.

I hope I'm wrong or just don't get the idea 👀

@mlc-mlapis
Copy link
Contributor

But it's not certain sure that parameter decorators won't be included in the final spec. There are still chances for it.

@yjaaidi
Copy link
Contributor Author

yjaaidi commented May 27, 2023

@amariq

The only alternative would be:

class A {
  private _something = inject(SOMETHING);
}

But, this could become possible in the future:

class A {
  constructor(private _something = inject(SOMETHING)) {}
}

@alxhub gave it a try but there were some "misuses" of default initializers that would break in that case. Cf. #48980 (comment)

About child overriding parent deps

That said, I think that child class B shouldn't override its parent constructor parameters for two reasons:

  1. class A might not want children to override its parameters
  2. parameters order is an implementation detail that would highly couple B to A.

If A wants to allow its child classes to override the dependencies without using DI, then you might need something different.
e.g.

class A {
  private _something = inject(SOMETHING);
  
  protected setSomething(something: Something) {
    // @todo throw an error if you don't want to allow this to be changed after A is instantiated.
    this._something = something;
  }
}

Of course, I would recommend relying on DI instead.

Dependency Injection vs Service Locator

Some might argue that using inject() is a service locator implementation and not dependency injection anymore.
IMO, what matters most is Inversion of Control more than the implementation detail. Cf. https://martinfowler.com/articles/injection.html

Harmonization

While this is not the main argument, inject() will soon be the only way of resolving dependencies in Router guards and HTTP interceptors. We still need more input, but I wonder if having a single way of resolving dependencies makes things easier or not for newcomers.

Usage with different builders

Using inject(), we don't need any build metadata and everything can be resolved at runtime with JIT.

You can give it a try here: https://github.com/yjaaidi/experiments/tree/angular-16

pnpm install
pnpm start --open # vite + angular esbuild
pnpm vite --open # vite + JIT

and compare the DX by yourself.

What do you think?

@yjaaidi
Copy link
Contributor Author

yjaaidi commented May 27, 2023

@mlc-mlapis true!

But let's consider both possibilities:
A. if parameter decorators happen, then the attribute decorator (which are not type-safe) become possible.
That said, if this PR happens before #48980, then it is type-safe and doesn't need the parameter decorators.

B. if parameter decorators don't happen and TypeScript 6 totally drops experimental decorators for example, then we are stuck and we'd quickly need to migrate. While some nice schematic can fix that, there could be some edge cases that won't work.
Also, even with a schematic, I see teams that prefer to migrate partially and slowly... due to a lack of confidence (caused by the absence of tests or a bad testing strategy etc... or manual migration habits)

Finally, as mentioned in my previous comment, inject() "survives" most compilers (e.g. vite without plugins, SWC etc...)

Cf. https://marmicode.io/blog/versatile-angular

@Jordan-Hall
Copy link

No please do not do this!

@yjaaidi
Copy link
Contributor Author

yjaaidi commented May 27, 2023

No please do not do this!

Hey @Jordan-Hall!
Can you elaborate please?
You think the deprecation is a bad idea?
Would this be an alternative that would suit you?
constructor(service = inject(Service))

@Jordan-Hall
Copy link

Jordan-Hall commented May 28, 2023

I think the standards are not yet finished, they a new proposal to expand on the current stage 3 decorators https://github.com/tc39/proposal-class-method-parameter-decorators.

I strongly believe that angular should use compile logic themselves to handle dependency injection i(if required) until we know what happens with decorators proposals.

One of the main reason I love angular is the use of the DI. Moving it to a function call would be horrible. The only way I would support this move, is if angular opens up the DI system for 3rd party replacement.

Also for your comment about surviving vite and other compilers. Most compiler for angular uses Ntsc under the hood or delegate alot to them. I paused my custom compiler with swc to work on a custom lexer parser for angular first (makes life easier). But adding the support for vite and swc for the compiler was super easy

@amariq
Copy link

amariq commented May 28, 2023

@yjaaidi

But, this could become possible in the future:

class A {
  constructor(private _something = inject(SOMETHING)) {}
}

@alxhub gave it a try but there were some "misuses" of default initializers that would break in that case. Cf. #48980 (comment)

Well, it's essentially what I was talking about but with the default initializers (forgot about them since I try not to use it mostly 😅), this would do! 👍
Thank you for referencing the #48980, it's good to know.

That said, I think that child class B shouldn't override its parent constructor parameters for two reasons:

  1. class A might not want children to override its parameters
  2. parameters order is an implementation detail that would highly couple B to A.

Can't agree with that, generally.

First of all, A can't do nothing to protect from children messing around with the deps while they are being injected thru the DI anyway, right?.

Second, if A doesn't want children to know its constructor arguments it could at least just use the Injector and get all the deps manually, thus allowing children to pass modified Injector to the parent if they want for their local needs. Looks similar to inject-ing, but better, imo. Still, I don't think it's good idea to hide what deps a class relies on.

Also don't really see a big problem with parameters order coupling B to A, honestly (we all use versioning for a reason 😉), but yeah, it can be limiting factor sometimes I guess.

If A wants to allow its child classes to override the dependencies without using DI, then you might need something different. e.g.

class A {
  private _something = inject(SOMETHING);
  
  protected setSomething(something: Something) {
    // @todo throw an error if you don't want to allow this to be changed after A is instantiated.
    this._something = something;
  }
}

Of course, I would recommend relying on DI instead.

Surely I was talking with the DI in mind.

The point is, if you inject into a property and child wants to pass something different/modified but only for personal use (that is, without overriding said token globally with the providers), then it would be 2 injections instead of just 1.

Also there are use cases when parent must be stopped from injecting something. For example, you create A based on B to add some new feature and/or provide better API to reduce boilerplate and whatnot. Meanwhile B injects C, but injecting B (or A) at certain time creates deps cycle due to some, lets say, poor decisions in C. To resolve this, you have to provide modified implementation of C, but to minimize the impact of this change it'd be best to only use it for A, meaning to pass it as parent constructor parameter. Or, in case of inject-ed properties, use some monstrosity with instantiation of A put in manually created Injector with C overrides thru providers inside the useFactory function. Doesn't sound fun, right? 😄

PS. I think it basically comes down to having a choice on how to do things, but lack of it makes things much harder than it should be in time of need.

@freshgum-bubbles
Copy link

As an interested bystander and someone who heavily uses DI, I'd like to make a few points.

The only alternative would be:

class A {
  private _something = inject(SOMETHING);
}

This is a really nasty anti-pattern, and the effects of it are exarcerbated by it being private: how would you override that in testing? I'm guessing you'd do A['_something'] = inject(MOCK_SOMETHING), but that's really nasty IMO.

Your code above is actually using something called the Service Locator pattern, which is
essentially where a service takes in a DI container and makes unknown requests against it.
This is widely regarded as an anti-pattern.

The main advantage of constructor parameter decoration is that all the dependencies
for the service are located in the constructor, so consumers of that service / module already
know what dependencies it would require.

When you move to the Service Locator pattern,
what you're effectively doing is obfuscating what the class actually requires, and setting up
strange constraints for consumers of the API: they'd have to make sure that SOMETHING
is available in the global injector prior to initializing the module (even though the usage of it can only be found in the implementation). Ouch!


class A {
  constructor(private _something = inject(SOMETHING)) {}
}

This is a good compromise, as the consumer can now see what injectables the service requires.
Personally, it may be better to wait until the ECMA proposal mentioned above goes further.
It would have been nice if TypeScript didn't remove such a widely-used feature, but oh well.

@alxhub gave it a try but there were some "misuses" of default initializers that would break in that case. Cf. #48980 (comment)

About child overriding parent deps

That said, I think that child class B shouldn't override its parent constructor parameters for two reasons:

  1. class A might not want children to override its parameters
  2. parameters order is an implementation detail that would highly couple B to A.

If A wants to allow its child classes to override the dependencies without using DI, then you might need something different. e.g.

class A {
  private _something = inject(SOMETHING);
  
  protected setSomething(something: Something) {
    // @todo throw an error if you don't want to allow this to be changed after A is instantiated.
    this._something = something;
  }
}

The main problem with the code above is that, should the constructor (or any methods it calls)
access _something before the extending class has a chance to call setSomething, consumers
of the API will end up with buggy behaviour, as demonstrated in this TypeScript Playground link.

Of course, I would recommend relying on DI instead.

Dependency Injection vs Service Locator

Some might argue that using inject() is a service locator implementation and not dependency injection anymore. IMO, what matters most is Inversion of Control more than the implementation detail. Cf. https://martinfowler.com/articles/injection.html

What matters is that consumers of a service understands its dependencies without having
to read through its source code, and then continually move other code around that when
the service is updated (and requires a new dependency).
Overall the solutions proposed seem half-baked, and it may be better just to wait until
there's further information from the TypeScript team (which I haven't found after a quick
search on their issue tracker) regarding the subject.

Harmonization

While this is not the main argument, inject() will soon be the only way of resolving dependencies in Router guards and HTTP interceptors. We still need more input, but I wonder if having a single way of resolving dependencies makes things easier or not for newcomers.

I don't think I'd be reaching to say most people use @Inject(). If they need specialized
behaviour, they'd use one of the more advanced decorators, e.g. @Self() or @SkipSelf().

Usage with different builders

Using inject(), we don't need any build metadata and everything can be resolved at runtime with JIT.

I'm not familiar with Angular's codebase, though I do note that their implementation of @Inject()
specifically requires that the subject token be declared, instead of inferring it through the
type of the parameter. This does make it a lot easier to implement, and most likely wouldn't
need much in the way of build-time metadata anyway (though happy to be corrected here).

What do you think?

I think holding off is the best solution right now. The solutions proposed seem half-baked,
and not to the standard one would expect from Angular. Furthermore, it may be wise to get
clarification from the TypeScript team regarding the ECMA specification mentioned above.

@Eraldo
Copy link

Eraldo commented Jun 17, 2023

Running into this challenge, as I rewrote my decorators to comply with the new decorator api.

Is there a workaround for @Optional()?

Simply removing it gives me errors where I have been using it before.
Example:

NullInjectorError: No provider for IonRouterOutlet!

I also tried:

private readonly ionRouterOutlet: IonRouterOutlet = inject(IonRouterOutlet),

=> Same error.

@JeanMeche
Copy link
Member

JeanMeche commented Jun 17, 2023

@Eraldo Arent't you looking for inject(IonRouterOutlet, { optional: true }) ?

@Eraldo
Copy link

Eraldo commented Jun 17, 2023

If I do this in my constructor:

  ...
  constructor(
    // Used to work before I disabled the experimentalDecorators:
    // @Optional() private ionRouterOutlet: IonRouterOutlet,
    private ionRouterOutlet: IonRouterOutlet = inject(IonRouterOutlet, { optional: true, })  // <- throws an error
  ) {}

I still get the same NullInjectorError error. 🤷

@JeanMeche
Copy link
Member

You can’t use inject as default value in the constructor. See #47606

The inject has to happen inside the body of the constructor.

@pkozlowski-opensource
Copy link
Member

This is a larger question of moving away from param decorators that would detailed design, RFC etc. - we track it in our backlog already and I don't think it is a good fit for an issue tracker.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime
Projects
None yet
Development

No branches or pull requests

9 participants