microsoft/tsyringe

InjectionToken type should allow abstract classes

Opened this issue ยท 7 comments

Abstract classes currently need symbols & annotations to be able to resolve in a constructor (similar to interfaces). IMO there's no reason for this limitation to exist, since the runtime knows about the type. The token shouldn't have the concrete constructor constraint at all for this case.

Example:

abstract class Logger {}
class ConsoleLogger extends Logger {}

@injectable()
class App {
  constructor(logger: Logger) {}
}
container.registerType(Logger, ConsoleLogger);
container.resolve(App);

This works perfectly if we remove the abstract keyword from the Logger class, but that isn't ideal since we don't want it to be instantiated.

It also works if we suppress the compiler error when registering the type:

Argument of type 'typeof Logger' is not assignable to parameter of type 'InjectionToken<Logger>'.
  Type 'typeof Logger' is not assignable to type 'constructor<Logger>'.
    Cannot assign an abstract constructor type to a non-abstract constructor type.ts(2345)

I recall we tried to implement this once before, but we found a bug where we couldn't prevent it from actually instantiating the abstract class (!) at runtime. Maybe this deserves another look.

We've just run in to this, but I believe there are specifics involved:

These don't work:

abstract class A {}

class B extends A {}

@injectable
class C {
  constructor(b: B) {}
}
abstract class A {
  constructor() {
    // do some stuff
  }
}

class B extends A {}

@injectable
class C {
  constructor(b: B) {}
}

These do work:

abstract class A {}

class B extends A {
  constructor() {
    // do some stuff
  }
}

@injectable
class C {
  constructor(b: B) {}
}
abstract class A {
  constructor() {
    // do some stuff
  }
}

class B extends A {
  constructor() {
    super();
    // possibly do something else
  }
}

@injectable
class C {
  constructor(b: B) {}
}

The reason for this is that if B doesn't have its own constructor, the constructor of A is used. This can't work for DI as we need to override the constructor, hence the problem trying to instantiate an abstract class. Even in the first example (the same situation as described by the OP) the constructor being used is implicitly that of A, which automatically has an empty one created. In that case B doesn't because A already has an empty one.

In fact, I'd recommend implementing constructors in all classes that extend an abstract class in Typescript, otherwise you can find unexpected side effects, where supposedly separate classes actually share the same prototype at run time.

pnkp commented

I have to mention that an abstract class can be used as an interface in TypeScript. Usually, I'm used to implementing abstract classes as interfaces for DI containers because it does not need to use "inject tokens" and @Inject decorator in class if you have registered those dependencies.
Here is an example:

export abstract class FileStorage {
  abstract put(inputData: unknown): Promise<void>;

  abstract get(fileName: string): Promise<string | undefined>;
}

export class ExampleStorage implements FileStorage {
  put(inputData: unknown): Promise<void> {
    throw new Error();
  }

  get(fileName: string): Promise<string | undefined> {
    throw new Error();
  }
}

container.register(FileStorage, { useClass: ExampleStorage });

@pnkp when trying your code, I still get Type typeof FileStorage' is not assignable to type 'constructor<FileStorage>.

but at runtime, I do get the ExampleStorage from container.resolve(FileStoreage).

I would like to register the abstract classes, and get my implementation, but without typescript complaining. I still dont get, why that should not work? Any explanation would be appreaciated :)

@pnkp when trying your code, I still get Type typeof FileStorage' is not assignable to type 'constructor<FileStorage>.

but at runtime, I do get the ExampleStorage from container.resolve(FileStoreage).

I would like to register the abstract classes, and get my implementation, but without typescript complaining. I still dont get, why that should not work? Any explanation would be appreciated :)

@Manuelbaun In my example above I just showed the problem ๐Ÿ˜„ It is not the solution

I am too using abstract classes as interfaces, just as @pnkp.
I ended up creating a simple utility function asImplementation, defined as:

import { InjectionToken, ClassProvider } from 'tsyringe';

type AbstractConstructor<T> = abstract new (...args: any[]) => T;
type ConcreteConstructor<T> = new (...args: any[]) => T;

export const asImplementation = <T>(
  InterfaceClass: AbstractConstructor<T>,
  ImplementationClass: ConcreteConstructor<T>
): [InjectionToken<T>, ClassProvider<T>] => {
  return [
    InterfaceClass as ConcreteConstructor<T>,
    {
      useClass: ImplementationClass,
    },
  ];
};

This is used as:

interface Engine {
  start(): void;
}

abstract class Engine {}

class V8Engine implements Engine {
  start(): void {
    // start the engine
  }
}

container.register(...asImplementation(Engine, V8Engine));

One problem with that is that it doesn't prevent the abstract class from being instantiated, as @MeltingMosaic mentioned.
One workaround I can think of is create a decorator like @interfaceOnly() that would override the abstract class constructor and prevent it from being instantiated in runtime.

Prevent abstract class from being instantiated

export const interfaceOnly = <
  TFunction extends abstract new (...args: any[]) => any
>() => {
  return (target: TFunction) => {
    return class {
      constructor(...args: any[]) {
        throw new Error(`Cannot instantiate ${target.name} interface`);
      }
    } as unknown as TFunction;
  };
};

Basic tests

import { container } from 'tsyringe';
import { asImplementation } from './asImplementation';
import { interfaceOnly } from './interfaceOnly';

describe('interfaces', () => {
  test('@interfaceOnly() throw error when trying to instantiate an abstract class acting as interface', () => {
    interface Engine {
      foo: string;
    }

    @interfaceOnly()
    abstract class Engine {}

    expect(() =>
      container.resolve(Engine as new (...args: any[]) => Engine)
    ).toThrowError();
  });

  test('asImplementation register implementation of an interface', () => {
    interface Engine {
      type: string;
    }

    @interfaceOnly()
    abstract class Engine {}

    class V8Engine implements Engine {
      type = 'V8';
    }

    container.register(...asImplementation(Engine, V8Engine));

    expect(
      container.resolve(Engine as new (...args: any[]) => Engine)
    ).toBeInstanceOf(V8Engine);
  });
});

I'd be happy to prepare a PR if that sounds like a good solution. Let me know!

The following solution seems to work, I don't see the abstract class being instanciated.

import 'reflect-metadata';
import { container, injectable, registry } from 'tsyringe';

type NonAbstractClass<T> = new (...args: unknown[]) => T;

let myAbstractClassIsInstanciated = false;
let myConcreteClassIsInstanciated = false;
let myOtherClassIsInstanciated = false;

abstract class MyAbstractClass {
  constructor() {
    myAbstractClassIsInstanciated = true;
  }
}

@injectable()
@registry([
  {
    token: MyAbstractClass as NonAbstractClass<MyAbstractClass>,
    useClass: MyConcreteClass,
  },
])
class MyConcreteClass implements MyAbstractClass {
  constructor() {
    myConcreteClassIsInstanciated = true;
  }
}

@injectable()
class MyOtherClass {
  constructor(private myAbstractClass: MyAbstractClass) {
    myOtherClassIsInstanciated = true;
  }
}

describe('tsyring dependency injection with abstract class', () => {
  it('should only instanciate MyConcreteClass', () => {
    myAbstractClassIsInstanciated = false;
    myConcreteClassIsInstanciated = false;
    myOtherClassIsInstanciated = false;
    container.resolve(MyAbstractClass as NonAbstractClass<MyAbstractClass>);
    expect(myAbstractClassIsInstanciated).toEqual(false);
    expect(myConcreteClassIsInstanciated).toEqual(true);
    expect(myOtherClassIsInstanciated).toEqual(false);
  });
  it('should instanciate MyConcreteClass and MyOtherClass', () => {
    myAbstractClassIsInstanciated = false;
    myConcreteClassIsInstanciated = false;
    myOtherClassIsInstanciated = false;
    container.resolve(MyOtherClass);
    expect(myAbstractClassIsInstanciated).toEqual(false);
    expect(myConcreteClassIsInstanciated).toEqual(true);
    expect(myOtherClassIsInstanciated).toEqual(true);
  });
});