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

First-class support for dependency decoration #40141

Closed
johncrim opened this issue Dec 15, 2020 · 10 comments
Closed

First-class support for dependency decoration #40141

johncrim opened this issue Dec 15, 2020 · 10 comments
Labels
area: core Issues related to the framework runtime core: di feature: insufficient votes Label to add when the not a sufficient number of votes or comments from unique authors feature: votes required Feature request which is currently still in the voting phase feature Issue that requests a new feature
Milestone

Comments

@johncrim
Copy link

johncrim commented Dec 15, 2020

🚀 feature request

Relevant Package

This feature request is for @angular/core

Description

Today it's clunky and verbose (and buggy if using jest + ViewEngine) to implement decoration of provided dependencies. Please consider adding first class support for decoration to the DI system. Doing so would make a number of use-cases cleaner and (probably) more efficient.

To clarify my use of the term "decoration" in this context, I mean: A function that takes an input injectable from an injector, and can either update or replace or wrap the input injectable, and returns an injectable that is used in place of the input injectable.

I do not mean typescript decorators (eg @MyDecorator()). Perhaps another term should be used to avoid confusion, but the term decoration seems appropriate as a well-known pattern.

Use cases include:

  1. Configuring injectables
  2. Replacing or wrapping default injectables in a component tree
  3. Providing an injectable for a component tree only if not provided by an ancestor

Describe the solution you'd like

I envision this feature working much like provider factory functions today, but adding the input injectable as a factory function argument. Decorator functions would be run after the object is initially created. In DI today, @SkipSelf() can be used to obtain an instance from a parent injector, but decorator functions should be able to run in the same injector where the object was created.

A provider factory function is specified today using:

function createFoo(): Foo {
  return new Foo();
}

@Directive({
  selector: 'my-directive',
  providers: [{
    provide: Foo,
    useFactory: createFoo
  }]
})
export class MyDirective ...

A decorator could be specified as follows:

function decorateFoo(input: Foo | null, ...args: any[]): Foo | null {
  if (input) {
     input.doSomething();
     return input;
  }
  else return new Foo();
}

@Directive({
  selector: 'my-directive',
  providers: [{
    provide: Foo,
    useDecorator: decorateFoo,
    deps: [ dependencies that are injected as args ]
  }]
})
export class MyDirective ...

The syntax shouldn't be limited to directives/components, it could also be used in modules.

Configuration use case

function disableAnimation(animationManager) {
  animationManager?.animationEnabled = false;
  return animationManager;
}

@NgModule({
  providers: [{
    provide: AnimationManager,
    useDecorator: disableAnimation
  }]
})
export class MyTestModule { }

In this example, AnimationManager is a normal @Injectable service, which would be created normally (its dependencies injected), and then passed through the decorator function before use.

Wrap injectable use case

function wrapDemoTheme(themeSettings) {
  const demoTheme = inject(DemoThemeSettings);
  demoTheme.currentTheme = themeSettings;
  return demoTheme;
}

@Component({
  providers: [{
    provide: ThemeSettings,
    useDecorator: wrapDemoTheme
  }]
})
export class DemoThemeComponent{  
  constructor(private readonly _themeSettings: ThemeSettings) {}
}

Provide if absent use case

In this use case, a PopupController instance provided by parent directives should be used, but if an instance doesn't exist one is provided by the component.

function ensurePopupController(inherited: PopupController | null): PopupController {
  if (inherited) {
    return inherited;
  }
  return new PopupController(inject(FocusManager));
}

@Component({
  providers: [{
    provide: PopupController,
    useDecorator: ensurePopupController
  }]
})
export class PopupComponent{ 
  constructor(private readonly _controller: PopupController) {}
}

Describe alternatives you've considered

I don't believe there are any good workarounds for decorating module providers, aside from using different injector tokens for the input and decorated output, and using a factory function for the output. If one were to use the same injector token for the input and decorated output, using InjectFlags.SkipSelf at the root injector would fail (would reach the null injector), and not using InjectFlags.SkipSelf would result an infinite loop.

There are 2 alternatives I've used for implementing decorator patterns in the element injector tree:

  1. Inject a component or directive with @SkipSelf() and provide the dependency to children - this works, but clutters up the directive with a lot of code, and the directive has to inject all the constructor parameters to create the provided dependencies.
  2. Use a factory function that injects the dependency with InjectFlags.SkipSelf, and returns the decorated version. This is closer to ideal, but is buggy today, due to spotty implementations of InjectFlags.SkipSelf.

Note that neither of these provide exactly what I'd like, as both use SkipSelf. As noted above, it would be preferable to be able to decorate an object without obtaining the decorator input from a parent injector.

Here's an example showing the first alternative (inject and provide the dependency in a directive):

@Component({
  selector: 'ui-popup',
  ...
  providers: [
    {
      provide: PopupController,
      useFactory: (c: PopupComponent) => c.controller,
      deps: [ PopupComponent ]
    },
    {
      provide: FOCUS_AREA,
      useFactory: (c: PopupComponent) => c.focusArea,
      deps: [ PopupComponent ]
    }
  ]
})
export class PopupComponent {

  /**
   * A @see PopupController that controls display of the popup. Created
   * on demand if not set or injected.
   */
  @Input()
  public get controller(): PopupController {
    if (!this._controller) {
      this._controller = new PopupController(this._focusManager);
    }
    return this._controller;
  }
  public set controller(p: PopupController) {
    if (p !== this._controller) {
      this._controller = p;
    }
  }
  private _controller?: PopupController;

  /** Returns the inherited or local FocusArea */
  public get focusArea(): FocusArea {
    return this._inheritedFocusArea ?? this._thisFocusArea;
  }
  private readonly _thisFocusArea?: FocusArea;

  constructor(
    private readonly _elt: ElementRef<HTMLElement>,
    private readonly _focusManager: FocusManager,
    @Inject(FOCUS_AREA) @Optional() @Host() @SkipSelf() inheritedFocusArea?: FocusArea,
    @Optional() @Host() @SkipSelf() popupController?: PopupController
  ) {
    if (!this._inheritedFocusArea) {
      this._thisFocusArea = new FocusArea(elt, _focusManager);
    }
    this._controller = popupController;
  }

 ...
}

This example component provides 2 objects to its children (a FocusArea and PopupController), if they aren't provided by ancestors. I've cut out all the code related to what the component does. Note that this works, but it adds a lot of unnecessary complexity to the component. All the dependencies needed to instantiate the FocusArea and PopupController wouldn't otherwise be needed in the component.

For the second alternative, I was able to create a more reusable method for providing missing dependencies (one of the decorator use-cases), but as described below it's problematic. Here's the logic for creating missing dependencies (returns a FactoryProvider):

/**
 * Like a `FactoryProvider`, but the factory function is only called if the
 * dependency is not already provided by a parent injector.
 */
export interface IfAbsentFactoryProvider<T> {
  /** The `Type<T>` or `InjectionToken<T>` to inject. */
  provide: Type<T> | InjectionToken<T>;
  /** A factory function to create a T */
  factory: (...args: any[]) => T;
  /** Injectable dependencies that are passed to the `factory` function. */
  deps?: any[];
}

/**
 * Injects an instance of type T if not already provided by a parent.
 *
 * @param provide A `Type<T>` or `InjectionToken<T>`
 * @param factory A factory function to create a T
 * @param deps Injectable dependencies that are passed to the `factory` function.
 */
export function provideIfAbsent<T>(
  p: IfAbsentFactoryProvider<T> | Type<T>): FactoryProvider {

  let provide: Type<T> | InjectionToken<T>;
  let factory: (...args: any[]) => T;
  let deps: any[] | undefined;
  if (typeof(p) === 'function') {
    provide = p as Type<T>;
    factory = () => new p();
  }
  else {
    provide = p.provide;
    factory = p.factory;
    deps = p.deps;
  }

  // See https://github.com/angular/angular/issues/31776
  // Injector.get() currently doesn't support InjectFlags.
  // inject() works in Ivy (only), which requires us to use karma for any tests that need Ivy
  // We can convert the tests back to Jest once this bug is fixed, or Jest supports Ivy. Grrr.
  let factoryDepth = 0;

  // Injector version:
  //function ifAbsentFactory(injector: Injector, ...args: any[]): T {

  function ifAbsentFactory(...args: any[]): T {
    if (factoryDepth === 0) {
      factoryDepth++; // Protect against recursion

      // See if there's an inherited instance of T
      const t: T | null = inject(provide, InjectFlags.Optional | InjectFlags.SkipSelf);
      // Injector version: = injector.get<T>(provide, undefined, InjectFlags.Optional | InjectFlags.SkipSelf);
      factoryDepth--;
      if (t !== null) {
        return t;
      }
    }

    // No inherited instance; use the factory function
    return factory(...args);
  }

  return {
    provide,
    useFactory: ifAbsentFactory,
    deps: deps // Injector version: ? [ Injector, ...deps ] : [ Injector ]
  };
}

And here's an equivalent example to the first showing how it can be used:

/** Provides a new FocusArea if one isn't already provided. */
export const inheritOrNewFocusArea= provideIfAbsent({
  provide: FOCUS_AREA,
  factory: (elt: ElementRef<HTMLElement>, fm: FocusManager) => new FocusArea(elt, fm),
  deps: [ElementRef<HTMLElement>, FocusManager]
});

/** Provides a new PopupController if one isn't already provided. */
export const inheritOrNewPopupController = provideIfAbsent({
  provide: PopupController,
  factory: (fm: FocusManager) => new PopupController(fm),
  deps: [FocusManager]
});

@Component({
  selector: 'ui-popup',
  template: '<ng-content></ng-content>',
  encapsulation: ViewEncapsulation.None,
  changeDetection: ChangeDetectionStrategy.OnPush,
  providers: [
    inheritOrNewFocusArea,
    inheritOrNewPopupController
  ]
})
export class PopupComponent {

  constructor(
    @Inject(FOCUS_AREA) private readonly _focusArea: FocusArea
    private readonly _controller: PopupController,
  ) {}
}

This is pretty close to what I'd like to see, but one issue is that it only works in Ivy ( inject() fails in ViewEngine, and InjectFlags.SkipSelf is not currently supported by node injector ). So, I'm not able to use our jest tests with it (for now) because jest compiles to ViewEngine (today).

Even though this second alternative is close, and is almost viable, I believe this is a feature worth adding, because it would cover all the use-cases.

Other comments

I think the expressiveness alone of something like useDecorator, given how frequently the pattern is used, would be nice for many code bases. It would also be very handy for test code. I acknowledge that provide: (token) may not be appropriate naming for decoration, I'm on the fence about that.

My guess is that first class support for dependency decoration could be more efficient than putting it all in factory functions, because it could be (partly or mostly) wired up by the compiler, reducing runtime code.

I suspect that it would enable many apps to be written with less code, enough to more than offset the addition to @angular/core.

@AndrewKushnir AndrewKushnir added area: core Issues related to the framework runtime core: di feature Issue that requests a new feature labels Dec 15, 2020
@ngbot ngbot bot added this to the Backlog milestone Dec 15, 2020
@mhevery
Copy link
Contributor

mhevery commented Dec 16, 2020

Let me see if I understand.

You would like to have a hook which runs after an injectable is created but before it is handed to whomever it is requesting. The hook can mutate the injectable instances (or ever replace it). Am I understanding it correctly?

NOTE: if we were to add such a feature it would be for Ivy only as VE is deprecated and will be removed soon.

@johncrim
Copy link
Author

johncrim commented Dec 16, 2020

@mhevery : Yes, that's correct.

I should add that I'd also like to be able to use the hook to create new objects if one doesn't exist (ie no provider currently exists in an ancestor element injector, so decorator input is undefined, and the hook can create + return one in that case). If you think that aspect of this feature is a bad idea, then a factory function with `SkipSelf | Optional' can be used.

@Airblader
Copy link
Contributor

If the hook mutated the injectable, that would also affect other places where it is injected, so this essentially performs a side effect that propagates up, down and laterally, doesn't it?

I suspect that it would enable many apps to be written with less code, enough to more than offset the addition to @angular/core.

I find it hard to imagine that this benefits enough people to offset the cost when an empty state template for ngFor doesn't, but don't mind my cynicism, I more than welcome additions to the DI since it's one of the benefits Angular has over other frameworks.

@johncrim
Copy link
Author

@Airblader : I don't see this proposal causing a new type of side-effects that could be problematic. Today any object receiving an injectable can mutate that injectable, causing the same. If you don't want that, make it immutable.

In the case where the decorator replaces the injectable, the replacement should take effect in the injector where the decorator is defined. So, the replacement would only be available in the view tree (for element injectors), or in the module scope (for module injectors).

@magicmatatjahu
Copy link

magicmatatjahu commented Jan 8, 2021

@johncrim Hello! Actually with Angular's version compatible with Ivy (from 9/10 version, I don't remember) you can do it using useFactory and with knowledge how DI system (and also compiler) in Angular works:

function useDecorator(provider: Type, decorate: (toDecoration: any, ...deps: any[]) => any, deps: any[]) {
  return {
    provide: provider,
    useFactory(...args: any[]) {
      // provider must be annotate by @Injectable() decorator so it works only with class provider
      // for useValue, useFactory, useExisting I think that currently is not possible, because
      // creating new provider by this helper we override old implementation for `provider` (argument of this function) token
      if (provider.hasOwnProperty("ɵfac")) {
        const toDecoration = provider["ɵfac"]();
        return decorate(toDecoration, ...args);
      } else {
        throw new Error(`${provider.name} is not injectable!`);
      }
    },
    deps,
  }
}

You can read this comment why this works: #13854 (comment) and in this comment you have also a example how to use existing provider from parent injector or create new one. I think that absent use case and decorator are completely different use cases :) And as I wrote in code snippet for useValue, useFactory, useExisting my proposition doesn't work. Cheers!

EDIT: I have an idea to handle also useValue, useFactory, useExisting, but first I want to test it :)

@johncrim
Copy link
Author

Thanks for the comment @magicmatatjahu . How would provider.ɵfac() fill in the Injectable's dependencies?

@magicmatatjahu
Copy link

magicmatatjahu commented Jan 12, 2021

@johncrim No problem.

How would provider.ɵfac() fill in the Injectable's dependencies?

Angular in compilation time (in JIT, and also in AOT mode) create from decorators like @Injectable() @Inject() etc... (decorators related to DI system) factory saved in ɵfac static property. You can see examples in angular tests https://github.com/angular/angular/blob/master/packages/core/test/di/r3_injector_spec.ts For example, this class in your app:

EDIT: If Angular won't find ɵfac then will look for ɵprov static property with factory field

@Injectable()
class Service {
  constructor(@Optional() readonly service: OptionalService|null) {}
}

by Angular's compilator is changed to (compilator removes decorators and create from them static property):

class Service {
  constructor(readonly service: OptionalService|null) {}

  static ɵfac = () => new Service(ɵɵinject(OptionalService, InjectFlags.Optional));
}

ɵɵinjec function imported from @angular/core has here https://github.com/angular/angular/blob/master/packages/core/src/di/injector_compatibility.ts#L77 source code and current implementation of this function is here. On each call ɵɵinject, Angular changes injector (context of injection) to read the value of given provider/type in the constructor, so it works like normal injection using Injector provider const value = injector.get(Service). In our case:

  • Angular check that must be created instance of Service class. Then change context of injection to nearest injector - for example. If you pass Service to AppModule in providers array then context of injection is AppModule.
  • Service needs OptionalService service so before creating Service instance, Angular must create OptionalService using ɵɵinject function etc...

Additionally. We cannot use Injector provider inside useFactory because Angular when read the module metadata override each token, so ɵfac is a workaround for that.

I know that it's a complicated but it works 😄 Let me know that everything works in your use case.

@angular-robot angular-robot bot added the feature: votes required Feature request which is currently still in the voting phase label Jun 4, 2021
@angular-robot
Copy link
Contributor

angular-robot bot commented Jun 4, 2021

Just a heads up that we kicked off a community voting process for your feature request. There are 20 days until the voting process ends.

Find more details about Angular's feature request process in our documentation.

@angular-robot
Copy link
Contributor

angular-robot bot commented Jun 25, 2021

Thank you for submitting your feature request! Looks like during the polling process it didn't collect a sufficient number of votes to move to the next stage.

We want to keep Angular rich and ergonomic and at the same time be mindful about its scope and learning journey. If you think your request could live outside Angular's scope, we'd encourage you to collaborate with the community on publishing it as an open source package.

You can find more details about the feature request process in our documentation.

@angular-robot angular-robot bot added the feature: insufficient votes Label to add when the not a sufficient number of votes or comments from unique authors label Jun 25, 2021
@jessicajaniuk jessicajaniuk closed this as not planned Won't fix, can't repro, duplicate, stale Jul 18, 2022
@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 18, 2022
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 core: di feature: insufficient votes Label to add when the not a sufficient number of votes or comments from unique authors feature: votes required Feature request which is currently still in the voting phase feature Issue that requests a new feature
Projects
None yet
Development

No branches or pull requests

6 participants