Removed Abstract<T> argument type from ServiceIdentifier cause BC break

helios-ag commented 1 week ago

Is there an existing issue for this?

[X] I have searched the existing issues

Current behavior

We use inversify with tsoa framework, and after recent update from 6.0.3 to 6.1.1 Inversify introduced BC break

Updated ServiceIdentifier to rely on Function instead of Abstract.

https://github.com/inversify/InversifyJS/blob/master/src/interfaces/interfaces.ts#L244

Tsoa use definition of container to be compatible with inversify's container get method

https://github.com/lukeautry/tsoa/blob/master/packages/runtime/src/interfaces/iocModule.ts#L2

Steps to reproduce

Upgrade from 6.0.x to 6.1.x And container can't be compiled due to incompatibility of signatures

Expected behavior

Container compiles as previously

Possible solution

Add Abstract<T> (or {prototype: T} which is the same) type to ServiceIdentifier in common package

Package version

6.1.0

Node.js version

22.11

In which operating systems have you tested?

[X] macOS
[ ] Windows
[X] Linux

Stack trace

No response

Other

notaphplover commented 1 week ago

Hey @helios-ag Please provide a minimum reproduction code of the issue.

helios-ag commented 1 week ago

hi @notaphplover sure: https://codesandbox.io/p/sandbox/inversify-tsoa-build-fail-7zldp4

output for npm run build:

 workspace git:(main) ✗ npm run build

> build
> tsoa spec-and-routes && tsc

tsoa-generated/routes.ts:48:23 - error TS2322: Type 'Container | IocContainer' is not assignable to type 'IocContainer'.
  Type 'Container' is not assignable to type 'IocContainer'.
    Types of property 'get' are incompatible.
      Type '<T>(serviceIdentifier: ServiceIdentifier<T>) => T' is not assignable to type '{ <T>(controller: { prototype: T; }): T; <T>(controller: { prototype: T; }): Promise<T>; }'.
        Types of parameters 'serviceIdentifier' and 'controller' are incompatible.
          Type '{ prototype: any; }' is not assignable to type 'ServiceIdentifier<any>'.
            Type '{ prototype: any; }' is missing the following properties from type 'Function': apply, call, bind, length, and 4 more.

48                 const container: IocContainer = typeof iocContainer === 'function' ? (iocContainer as IocContainerFactory)(request) : iocContainer;
                         ~~~~~~~~~

Found 1 error in tsoa-generated/routes.ts:48

notaphplover commented 1 week ago

Ok, I see the issue here. Basically, tsoa has the following IocContainer:

export interface IocContainer {
  get<T>(controller: { prototype: T }): T;

  get<T>(controller: { prototype: T }): Promise<T>;
}

Since ServiceIdentifier doesn't have Abstract<T>, it's no longer compatible.

As said in other issues, there're good reasons to remove Abstract<T>.

Let's see an ancient code example:

interface Foo {
  bar: string;
}

function MyAncientJavscriptClass(this: Foo): void {
  this.bar = 'baz';
}

MyAncientJavscriptClass.prototype.getBar = function (this: Foo): string {
  return this.bar;
};

if we ask typescript which is MyAncientJavscriptClass.prototype type, typescript will answer with any because function prototypes are any, but the real prototype contract would be something like:

interface MyAncientJavscriptClassProto {
  getBar(): string;
}

The prototype does not expose any of Foo properties, Abstract<T> is just a trick to exploiting function prototypes are typed as any. Any intended Abstract<T> is a Function as well, objects with a prototype property are not supposed to be accepted by Abstract<T>. For such a reason Abstract<T> was removed because interfaces.ServiceIdentifier<T> is a service identifier of a service of type T, and Abstract does not respect this rule.

In inversify@6.0.3:

container.bind({ prototype: { foo: 'bar' } }).toSelf();

is inferring T as interfaces.ServiceIdentifier<{ foo: string; }> which is clearly wrong.

I would suggest you to ask in the tsoa repository. They can either update their container or patch their generated source code.

I'll close the issue now, but feel free to continue discussing in this thread. If there's something I'm missing I am open to change my mind and reconsider my position, but the way I see this, replacing Abstract<T> is a bugfix, not a breaking change.

helios-ag commented 1 week ago

hi @notaphplover I agree with your arguments here about abstract type, but removing it from library which exposes its public api, and consequentually changed its behavior, seems major change, which breaks semver approach

notaphplover commented 1 week ago

Hey @helios-ag, interfaces.Abstract was not removed. I replaced interfaces.Abstract from interfaces.ServiceIdentifier.

All the bug fixes are breaking changes, they alter the code base so a wrong behavior becomes the expected behavior. When I put on your shoes I can understand your position, it can be really frustrating to bump a library version and find these not so pleasant surprises. But this kind of special breaking changes trigger patch updates instead of major ones.

On the other hand, I do believe this is a bugfix. Abstract<T> was introduced in interfaces.ServiceIdentifier without ever expecting to accept objects or to exploit the any nature of a prototype function. Consider this page as reference. It's not necessarily an absolute source of truth, but I find it useful at these times and makes a lot of sense to me.

helios-ag commented 1 week ago

hi @notaphplover, got your position here related to Abstract, it seems not used anywhere in the lib after that change.

inversify / InversifyJS