inversify / InversifyJS

A powerful and lightweight inversion of control container for JavaScript & Node.js apps powered by TypeScript.
http://inversify.io/
MIT License
10.97k stars 713 forks source link

Type-safe decorators? #788

Open JoshuaKGoldberg opened 6 years ago

JoshuaKGoldberg commented 6 years ago

Consider this syntactically correct code:

@injectable()
class Sample {
    @inject(InjectionTypes.Dependency) // where IT.D is actually a class
    dependency: number;
}

I've made subtle mistakes around this enough to be emotionally invested in finding way to use the type system to find this class of mistake.

Is it at all possible to add an optional (= any) type onto the @injectable and/or @inject decorators? For example, something like:

type MyInjections = {
    Dependency: typeof Dependency;
};
@injectable<MyTypes>()
class Sample {
    @inject(InjectionTypes.Dependency)
    dependency: number;
             // ~~~~~~ Error: expected type to be typeof Dependency
}

...or...

@injectable()
class Sample {
    @inject<MyTypes>(InjectionTypes.Dependency)
    dependency: number;
             // ~~~~~~ Error: expected type to be typeof Dependency
}

I've previously done something smaller and less difficult to reason about this in another package, which might be useful as a frame of reference.

remojansen commented 6 years ago

Hi @JoshuaKGoldberg I don't know if I'm getting you. Is InjectionTypes.Dependency is a class @inject(InjectionTypes.Dependency) should work with:

container.bind<typeof InjectionTypes.Dependency>(InjectionTypes.Dependency).toSelf();

But based on your second example:

type MyInjections = {
    Dependency: typeof Dependency;
};

I'm wondering if InjectionTypes.Dependency is a class? because typeof Dependency is not a class.

If using a class maybe the problem is that when @inject(InjectionTypes.Dependency) is invoked (when the class declaration is executed) maybe it is likely to be too early for InjectionTypes.Dependency to be declared. The latest release has a fix for this called LazyServiceIdentifer:

import { LazyServiceIdentifer, inject } from "inversify";

// ...

@inject(new LazyServiceIdentifer(() => InjectionTypes.Dependency))

You can also declare a helper:

const lazyEvaluatedInject = (id: any) =>  inject(new LazyServiceIdentifer(() =>id));

// ...

@lazyEvaluatedInject(InjectionTypes.Dependency)
JoshuaKGoldberg commented 6 years ago

The issue I'm trying to solve is that there's no guarantee at development time that we correctly hook up our injections. Giving a more complete example:

import { Container, inject, injectable } from "inversify";

@injectable()
class Dependency { }

@injectable()
class Sample {
    @inject(Dependency)
    dependency: number; // this is clearly wrong
}

const container = new Container();

container.bind(Dependency).toSelf();
container.bind(Sample).toSelf();

const sample = container.get(Sample);
alecgibson commented 6 days ago

I know this is an old issue, but was looking for a place to document the fact that we've got this working and I'm actually quite happy with our setup now.

It works pretty well. The main issues are:

Hopefully this will help someone!

TypedContainer

import {interfaces} from 'inversify';

type StringKey<T> = keyof T & string;

type TypedContainerBase<T extends Record<string, any>> = {
  bind: <K extends StringKey<T>, B extends T[K]>(serviceIdentifier: K) => interfaces.BindingToSyntax<B>;
  rebind: <K extends StringKey<T>, B extends T[K]>(serviceIdentifier: K) => interfaces.BindingToSyntax<B>;
  get: <K extends StringKey<T>, B extends T[K]>(serviceIdentifier: K) => B;

  // Other overrides as needed
};

export type TypedContainer<T extends Record<string, any>> = TypedContainerBase<T> &
Omit<interfaces.Container, keyof TypedContainerBase<T>>;

$getDecorators()

import {inject, multiInject} from 'inversify';

type StringKey<T> = keyof T & string;
type TypedDecorator<T> = <O extends Record<K, T>, K extends keyof O>(
  target: O,
  targetKey?: K,
  indexOrPropertyDescriptor?: any,
) => void;
type InjectDecorator<T> = <K extends StringKey<T>>(binding: K) => TypedDecorator<T[K]>;

export interface IDecorators<T> {
  $inject: InjectDecorator<T>;
  // Other decorators as needed
}

export function $getDecorators<T extends Record<string, any>>(): IDecorators<T> {
  const $inject: InjectDecorator<T> = (binding) => {
    return inject(binding) as ReturnType<InjectDecorator<T>>;
  };

  return {
    $inject,
  };
}

Define bindings type

export type Bindings = {
  Foo: IFoo;
  Bar: IBar;
}

export const {$inject} = $getDecorators<Bindings>();

Define container

export const container: TypedContainer<Bindings> = new Container();

container.bind('Foo').to(Foo); // This is now strongly-typed

Using bindings

@injectable()
public class Test {
  @$inject('Foo')
  public readonly foo: IFoo; // strongly typed!

  @$inject('Bar')
  public readonly bar: IFoo; // uh-oh: compiler error, should be `IBar`
}
PodaruDragos commented 6 days ago

@alecgibson this is really cool, maybe there is a way we can add this to inversify itself, maybe with a new decorator typedInject or just do a breaking change and change the inject itself. I don't really know how hard that would be but i would assume it's not that trivial. Anyways if anyone want to PR this I would be happy to get it merged.

alecgibson commented 6 days ago

@PodaruDragos I can try to have a look at getting this into inversify itself. I think it needs to be a separate decorator, because my special @$inject() decorator doesn't let you do private property injection, which is a valid pattern that some people may be using @typedInject() sounds like a sensible name.

I think I can probably make the Container strongly-typed in a non-breaking way, but I haven't look properly yet. Will report back.

alecgibson commented 6 days ago

@PodaruDragos I've raised https://github.com/inversify/InversifyJS/pull/1575 as a first part of this change: just adding strong types to Container (in itself quite a big change!). Once that's in, I can look into strong typing for injections as a follow-up.

PodaruDragos commented 6 days ago

I really like that approach. Want to get more opinions on this from members though. @notaphplover, @dcavanagh @jakehamtexas what do you guys think about this ?