ServiceIdentifier is invariant due to addition of Function

[kenneth-devedis]

Is there an existing issue for this?

• I have searched the existing issues

Current behavior

The ServiceIdentifier Type was changed from: string | symbol | Newable<T> to string | symbol | Newable<T> | Function.

I think the issue here is that Function is a super type of Newable<T> and makes ServiceIdentifier invariant to its generic type argument.

This means that the following typescript code is valid:

class SomeClass {}

const barString = 'bar' as ServiceIdentifier<number>
const barSymbol = Symbol('bar') as ServiceIdentifier<number>
const barNewable = Number
const wrongNewable: ServiceIdentifier<string> = SomeClass

let foo: ServiceIdentifier<string>
foo = barString
foo = barSymbol
foo = barNewable
foo = wrongNewable

let fooTuple: [ServiceIdentifier<string>]
fooTuple = [barString]
fooTuple = [barSymbol]
fooTuple = [barNewable]
fooTuple = [wrongNewable]

This also means that BindToFluentSyntax.toResolvedValue is actually not typesafe.

Steps to reproduce

1. Paste the above code snipping into any project using inversify
2. type-check using tsc

Expected behavior

the above code snippet causes typescript to fail.

Possible solution

Replace the Function in the ServiceIdentifier union type, with something more typesafe.

export type TypedFunction<TInstance = unknown, TArgs extends unknown[] = any[]> = (...args: TArgs) => TInstance;


type ServiceIdentifier<TInstance = unknown> = string | symbol | Newable<TInstance> | TypedFunction<T>;

Not sure if this is considered a breaking change.

Package version

7.5.1

Node.js version

22.16.0

In which operating systems have you tested?

• macOS
• Windows
• Linux

Stack trace

No response

Other

No response

[notaphplover]

Hey @kenneth-devedis , this is the expected behavior. This change was done in inversify@6 as a fix.

The ServiceIdentifier Type was changed from: string | symbol | Newable<T> to string | symbol | Newable<T> | Function.

I don't think this is accurate, it was change from string | symbol | Newable<T> | Abstract<T> to string | symbol | Newable<T> | Function.

This is the expected behavior because a function can be used as a service identifier for any value. Consider this code sample:

const container = new Container();

container.bind(class {}).toConstantValue('my-sample-value');

Thus, a class (which is really a function) is used as service identifier for a string value the same way a symbol can be used as a service identifier for any kind of value.

This also means that BindToFluentSyntax.toResolvedValue is actually not typesafe

Type safety cannot be guaranteed since the type of the resolved value of a service is not necessarily inferred from its service identifier. I don't know how to workaround this, your proposal would be a breaking change instead of a fix since the current behavior is expected.

I'm closing the issue but feel free to continue the discussion, I am more than happy to participate and reply.

[kenneth-devedis]

@notaphplover Thanks for the clarification and quick answer ❤️.

I agree, that ServiceIdentifier does not provide actual type safety, but it helps by preventing misconfiguration.

My only point is, that the introduction of Function to ServiceIdentifier changed the behaviour of the type regarding typescript (quite drastically in my opinion).

It basically makes all ServiceIdentifiers assignable to each other regardless of their generic subtype. This wasn't the case with the previous version.

This compiles in 6.1.0, but does not compile in 6.0.3:

import {interfaces} from 'inversify';

type ServiceIdentifier<T> = interfaces.ServiceIdentifier<T>;

class FooService {
    foo() {
        console.log('foo');
    }
}

class BarService {
    bar() {
        console.log('bar');
    }
}

let fooIdentifier: ServiceIdentifier<FooService> = BarService
let barIdentifier: ServiceIdentifier<BarService> = FooService
fooIdentifier = barIdentifier
barIdentifier = fooIdentifier

Not sure if that was intentionally.

[notaphplover]

Hey @kenneth-devedis 🙂,

My only point is, that the introduction of Function to ServiceIdentifier changed the behaviour of the type regarding typescript (quite drastically in my opinion).

Function was a replacement for Abstract<T>, which was the following interface:

export interface Abstract<T> {
    prototype: T;
}

Which was incorrect for a variety of reasons. Some of them are exposed here. As a result, Function was the lesser evil.

Removing Function ould lead to abstract classes not being able to be service identifiers. Consider the following sample code:

export type Newable<T> = new (...args: any[]) => T;
export type ServiceIdentifier<T = unknown> = (string | symbol | Newable<T>);

abstract class A {}

const aServiceIdentifier: ServiceIdentifier<A> = A;

tsc complains with the following error:

'aServiceIdentifier' is declared but its value is never read.(6133)
Type 'typeof A' is not assignable to type 'ServiceIdentifier<A>'.
  Type 'typeof A' is not assignable to type 'Newable<A>'.
    Cannot assign an abstract constructor type to a non-abstract constructor type.(2322)

I would be happy to go for a better approach, if you have any suggestion it's more than welcome. Unfortunatelly, the one you proposed doesn't seem to be good enough:

export type Newable<T> = new (...args: any[]) => T;
export type TypedFunction<TInstance = unknown, TArgs extends unknown[] = any[]> = (...args: TArgs) => TInstance;

export type ServiceIdentifier<T = unknown> = (string | symbol | Newable<T> | TypedFunction<T>);

abstract class A {}

const aServiceIdentifier: ServiceIdentifier<A> = A;

tsc complains for the same reason:

'aServiceIdentifier' is declared but its value is never read.(6133)
Type 'typeof A' is not assignable to type 'ServiceIdentifier<A>'.
  Type 'typeof A' is not assignable to type 'Newable<A>'.
    Cannot assign an abstract constructor type to a non-abstract constructor type.(2322)

[kenneth-devedis]

Depends on how backwards compatible you want to be, you could use Abstract Constructor Signatures: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-2.html#abstract-construct-signatures

type Newable<T> = abstract new (...args: unknown[]) => T

Like this Newable would accept concrete and abstract classes.

class FooService {
    foo() {
        console.log('foo');
    }
}

abstract class BarService {
    bar() {
        console.log('bar');
    }
}

type BazService =  {
    baz(): void
}

const createBazService = () => {
    return {
        baz() {
            console.log('baz');
        }
    }
}
type SafeFunction<TInstance = unknown, TArgs extends unknown[] = unknown[]> = (...args: TArgs) => TInstance;
type Newable<TInstance = unknown, TArgs extends unknown[] = unknown[]> = abstract new (...args: TArgs) => TInstance;

type ServiceIdentifier<T> = Newable<T> | SafeFunction<T>;

const fooService: ServiceIdentifier<FooService> = FooService;
const barService: ServiceIdentifier<BarService> = BarService;
const bazService: ServiceIdentifier<BazService> = createBazService;

[notaphplover]

Hey @kenneth-devedis,

Depends on how backwards compatible you want to be, you could use Abstract Constructor Signatures

Oh, you're right. Yeah, I would prefer not to change Newable, but an AbstractNewable type could be added so ServiceIdentifier<T> is (string | symbol | Newable<T> | AbstractNewable<T>).

I will give it a loop in order to release a fix for both inversify@6 and inversify@7.

[notaphplover]

Hey @kenneth-devedis, It's already done in inversify@7. I'll release a new inversify@6 version this week with the fix

[ChristiaanScheermeijer]

I came here because I'm getting the same typescript error after upgrading from 7.1.0 to 7.5.3. It's debatable if this was a breaking change. But the reason I'm commenting is because our abstract classes (that serve as the identifiers as well) have protected constructors.

Long story short, abstract classes with a protected constructor won't comply to AbstractNewable<T>, but that might be expected.

https://github.com/jwplayer/ott-web-app/blob/develop/packages/common/src/services/integrations/AccountService.ts#L41-L51

[notaphplover]

Hey @ChristiaanScheermeijer , I see. Then I'll publish a patch to rollback those changes and deprecate the version. I'll add additional tests to make sure I never make the same mistake again

[ChristiaanScheermeijer]

Hi @notaphplover, no worries. I don't think a patch is needed just yet, I already fixed it in our project by removing the protected statement.

Thank you for your great work on Inversify! 😄

[notaphplover]

Hey @ChristiaanScheermeijer 🙂 ,

Thanks man, but I honestly believe I must roll it back. AbstractNewable can stay, it doesn't hurt and it helps with type inference, but users should be able to use classes as service identifiers regardless of the visibility of its constructor. Thanks for your kind words :)