feat: strongly-typed `Container` #1575

alecgibson · 2024-07-05T12:02:22Z

Description

This is a non-breaking change, affecting only TypeScript types, and doesn't change the implementation in any way.

Motivation

inversify already has some basic support for types when binding, and retrieving bindings.

However, the type support requires manual intervention from developers, and can be error-prone.

For example, the following code will happily compile, even though the types here are inconsistent:

container.bind<Bar>('bar').to(Bar);
const foo = container.get<Foo>('bar')

Furthermore, this proposal paves the way for type-safe injection, which will be added once this change is in.

Improved type safety

This change adds an optional type parameter to the Container, which takes an identifier map as an argument. For example:

type IdentifierMap = {
  foo: Foo;
  bar: Bar;
};

const container = new Container<IdentifierMap>;

If a Container is typed like this, we now get strong typing both when binding, and getting bindings:

const container = new Container<IdentifierMap>;

container.bind('foo').to(Foo); // ok
container.bind('foo').to(Bar); // error

const foo: Foo = container.get('foo') // ok
const bar: Bar = container.get('foo') // error

This also has the added benefit of no longer needing to pass around service identifier constants: the strings (or symbols) are all strongly typed, and will fail compilation if an incorrect one is used.

Non-breaking

This change aims to make no breaks to the existing types, so any Container without an argument should continue to work as it did before.

Related Issue

#788

How Has This Been Tested?

added tests to this test suite, which passes
based on type definitions we've been using in Production ourselves

Types of changes

Updated docs / Refactor code / Added a tests case (non-breaking change)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
I have updated the changelog.

alecgibson · 2024-07-05T13:37:32Z

src/interfaces/interfaces.ts

  }

-  export type Bind = <T = unknown>(serviceIdentifier: ServiceIdentifier<T>) => BindingToSyntax<T>;
+  export type Bind<T extends BindingMap = any> =
+    <B extends ContainerBinding<T, K>, K extends ContainerIdentifier<T> = any>(serviceIdentifier: K) => BindingToSyntax<B>;


Note, in the interest of keeping changes to a minimum, I haven't touched the BindingToSyntax type, but we may want to consider tweaking it — either as part of this change, or a future change — because:

container.bind('foo').toDynamicValue( ({container}) => container.get('bar') // not strongly typed )

alecgibson · 2024-08-20T12:04:11Z

@PodaruDragos any thoughts on this?

PodaruDragos · 2024-10-23T10:16:53Z

this looks really good to me, @notaphplover what do you think ?

notaphplover · 2024-10-23T10:29:54Z

this looks really good to me, @notaphplover what do you think ?

I need time to review this :). The idea is brilliant, I'll give my feedback once I review it.

notaphplover

This PR is sort of challenging to review. I would love to discuss some concepts before continue reviewing.

Overal, the PR looks amazing, I think it's going to provide so much value to the container!

notaphplover · 2024-10-23T12:37:19Z

src/interfaces/interfaces.ts

-  export type ServiceIdentifier<T = unknown> = (string | symbol | Newable<T> | Abstract<T>);
+  export type IfAny<T, Y, N> = 0 extends (1 & T) ? Y : N;
+
+  export type PropertyServiceIdentifier = (string | symbol);


This name is sort of misleading. A property can be decorated with any ServiceIdentifier. Is this a BindingMapProperty

notaphplover · 2024-10-23T12:39:38Z

src/interfaces/interfaces.ts

+  export type IfAny<T, Y, N> = 0 extends (1 & T) ? Y : N;
+
+  export type PropertyServiceIdentifier = (string | symbol);
+  export type ServiceIdentifier<T = unknown> = (PropertyServiceIdentifier | Newable<T> | Abstract<T>);


notaphplover · 2024-10-23T12:43:40Z

src/interfaces/interfaces.ts

+  export type PropertyServiceIdentifier = (string | symbol);
+  export type ServiceIdentifier<T = unknown> = (PropertyServiceIdentifier | Newable<T> | Abstract<T>);
+  export type BindingMap = Record<PropertyServiceIdentifier, any>;
+  export type BindingMapKey<T extends BindingMap> = keyof T & PropertyServiceIdentifier;


This feels redundant. keyof T & PropertyServiceIdentifier should be equivalent to keyof T since T is enforced to have an index of PropertyServiceIdentifier. Am I missing something?

If so, lets remove this type in favor of keyof usages

I agree it feels redundant, but if you try to change:

-export type ContainerIdentifier<T extends BindingMap> = IfAny<T, ServiceIdentifier, BindingMapKey<T>>; +export type ContainerIdentifier<T extends BindingMap> = IfAny<T, ServiceIdentifier, keyof T>;

Then you get a slew of compilation errors, along the lines of:

src/container/container.ts:181:33 - error TS2345: Argument of type 'string | number | symbol | Newable<unknown> | Abstract<unknown>' is not assignable to parameter of type 'ServiceIdentifier<unknown>'. Type 'number' is not assignable to type 'ServiceIdentifier<unknown>'. 181 this._bindingDictionary.add(serviceIdentifier, binding as Binding<unknown>);

You can see from the error that number has snuck in as a possible identifier, which ServiceIdentifier doesn't allow.

I think this is from the possibility that the BindingMap could be set to any (if no map is provided, which is the backwards-compatible case). any can technically have number as a key, hence the error.

We need to keep the any possibility to allow for containers that use non-string or symbol identifiers, (eg class identifiers) which is incompatible with this BindingMap approach, since classes can't be property keys.

If you can find a way to get this to compile without this, then I'm all ears, but this definitely feels like the simplest solution to me.

notaphplover · 2024-10-23T12:49:23Z

src/interfaces/interfaces.ts

@@ -46,7 +46,16 @@ namespace interfaces {
    prototype: T;
  }

-  export type ServiceIdentifier<T = unknown> = (string | symbol | Newable<T> | Abstract<T>);
+  export type IfAny<T, Y, N> = 0 extends (1 & T) ? Y : N;


Really smart, but sort of tricky. Tbh I don't see a better way to represent this.

notaphplover · 2024-10-23T12:57:38Z

src/interfaces/interfaces.ts

+  export type ServiceIdentifier<T = unknown> = (PropertyServiceIdentifier | Newable<T> | Abstract<T>);
+  export type BindingMap = Record<PropertyServiceIdentifier, any>;
+  export type BindingMapKey<T extends BindingMap> = keyof T & PropertyServiceIdentifier;
+  export type ContainerIdentifier<T extends BindingMap> = IfAny<T, ServiceIdentifier, BindingMapKey<T>>;


Suggested change

export type ContainerIdentifier<T extends BindingMap> = IfAny<T, ServiceIdentifier, BindingMapKey<T>>;

export type MappedServiceIdentifier<T extends BindingMap> = IfAny<T, ServiceIdentifier, BindingMapKey<T>>;

It feels more a mapped service identifier, isn't it? I think we don't need to involve the concept of container.

notaphplover · 2024-10-23T13:03:39Z

src/interfaces/interfaces.ts

+  export type BindingMap = Record<PropertyServiceIdentifier, any>;
+  export type BindingMapKey<T extends BindingMap> = keyof T & PropertyServiceIdentifier;
+  export type ContainerIdentifier<T extends BindingMap> = IfAny<T, ServiceIdentifier, BindingMapKey<T>>;
+  export type ContainerBinding<T extends BindingMap, K extends ContainerIdentifier<T> = any> = K extends keyof T ? T[K] :


I know I'm super picky with names, sry about it. I understand this is the service type of a binding, but not a binding. Am I right?

No worries. Naming stuff is the hardest part of the job! It's especially tricky because I'm not super well-versed in this library, so have probably just been naming stuff wrong.

This type ContainerBinding<T, K> is meant to represent the return value type of container<T>.get(key: K): ContainerBinding<T, K>. So I guess it's whatever you want to call that?

Then I guess it's a ServiceType (BindingMapServiceType would be too long, wouldn't it be?)

notaphplover · 2024-10-23T13:06:32Z

test/interfaces/interfaces.test.ts

@@ -0,0 +1,124 @@
+import { interfaces } from '../../src/interfaces/interfaces';


❤️

Thanks for adding this test module

This is a non-breaking change, affecting only TypeScript types, and doesn't change the implementation in any way. Motivation ========== `inversify` already has some basic support for types when binding, and retrieving bindings. However, the type support requires manual intervention from developers, and can be error-prone. For example, the following code will happily compile, even though the types here are inconsistent: ```ts container.bind<Bar>('bar').to(Bar); const foo = container.get<Foo>('bar') ``` Furthermore, this paves the way for [type-safe injection][1], which will be added once this change is in. Improved type safety ==================== This change adds an optional type parameter to the `Container`, which takes an identifier map as an argument. For example: ```ts type IdentifierMap = { foo: Foo; bar: Bar; }; const container = new Container<IdentifierMap>; ``` If a `Container` is typed like this, we now get strong typing both when binding, and getting bindings: ```ts const container = new Container<IdentifierMap>; container.bind('foo').to(Foo); // ok container.bind('foo').to(Bar); // error const foo: Foo = container.get('foo') // ok const bar: Bar = container.get('foo') // error ``` This also has the added benefit of no longer needing to pass around service identifier constants: the strings (or symbols) are all strongly typed, and will fail compilation if an incorrect one is used. Non-breaking ============ This change aims to make no breaks to the existing types, so any `Container` without an argument should continue to work as it did before. [1]: inversify#788 (comment)

PodaruDragos · 2024-10-24T09:53:57Z

src/interfaces/interfaces.ts

+
+  export type PropertyServiceIdentifier = (string | symbol);
+  export type ServiceIdentifier<T = unknown> = (PropertyServiceIdentifier | Newable<T> | Abstract<T>);
+  export type BindingMap = Record<PropertyServiceIdentifier, any>;


is there any way we can have this as Record<PropertyServiceIdentifier, unknown>; ? avoiding the any there i mean

What's the benefit? I think any is more semantic here?

no real benefit, keep consistency with most of the types since I tried really hard at some point to move away from most of any's and have "proper" types

I had a quick go, and it didn't immediately compile. I'm not personally sure it's worth investigating, because as I say the semantics aren't really "correct" (according to my limited understanding of unknown vs any 😅 ), since the values assigned in this BindingMap will be entirely consumer-generated, and they can legitimately set it to anything they want (ie it is semantically any, right...?).

If you feel strongly I can try to investigate, but the types don't really like it as-is:

src/container/container.ts:287:41 - error TS2345: Argument of type 'this' is not assignable to parameter of type 'Container<BindingMap, any>'. Type 'Container<T, P>' is not assignable to type 'Container<BindingMap, any>'. The types returned by 'bind(...)' are incompatible between these types. Type 'BindingToSyntax<ContainerBinding<T, K>>' is not assignable to type 'BindingToSyntax<B>'. Type 'ContainerBinding<T, K>' is not assignable to type 'B'. 'ContainerBinding<T, K>' is assignable to the constraint of type 'B', but 'B' could be instantiated with a different subtype of constraint 'unknown'. Type 'ContainerBinding<T, string> | ContainerBinding<T, symbol>' is not assignable to type 'B'. 'ContainerBinding<T, string> | ContainerBinding<T, symbol>' is assignable to the constraint of type 'B', but 'B' could be instantiated with a different subtype of constraint 'unknown'. Type 'ContainerBinding<T, string>' is not assignable to type 'B'. 'ContainerBinding<T, string>' is assignable to the constraint of type 'B', but 'B' could be instantiated with a different subtype of constraint 'unknown'. Type 'T[keyof T & string]' is not assignable to type 'B'. 'T[keyof T & string]' is assignable to the constraint of type 'B', but 'B' could be instantiated with a different subtype of constraint 'unknown'. Type 'T[string]' is not assignable to type 'B'. 'T[string]' is assignable to the constraint of type 'B', but 'B' could be instantiated with a different subtype of constraint 'unknown'. 287 const request = createMockRequest(this, serviceIdentifier, key, value);

I don't really think you can go around this tbh. If you can that would be amazing. depends what @notaphplover thinks also

- `PropertyServiceIdentifier` -> `BindingMapProperty` - `ContainerIdentifier` -> `MappedServiceIdentifier` - revert `ServiceIdentifier` type to previous definition

notaphplover · 2024-10-24T17:53:39Z

Hey @alecgibson I've been analizying this with @rsaz for long time.

We've been discussing pros and cons of this approach and which would be the logical enhancements once this strong typed container is ready.

Even if it's a great idea, we do believe inversify shouldn't be the place to include this enhancement. Would you consider extracting a plugin? I wouldn't mind adding a reference to the plugin in the docs.

If you want we can have a discussion through discord or any other channel so we can have a videocall and
exchange our different points of view :)

alecgibson · 2024-10-25T07:22:39Z

Even if it's a great idea, we do believe inversify shouldn't be the place to include this enhancement. Would you consider extracting a plugin?

A plugin is a legitimate architectural decision, although I am curious to hear your reasoning why? The way I see it, this adds a nice bit of backwards-compatible enhancement with basically no additional bloat. The main downsides I can think of are:

more complicated type definitions to maintain (but that's kind of the point of the change)
constrains other possible future use of a generic type signature on Container (if that matters?)

On the other hand, moving to a plugin comes with all the downsides of plugins:

increased maintenance and general overheads of running and maintaining a second repo/package
increased friction for consumers to use it (need to find it, and figure out how to install/use it, and it's yet another dependency)
less discoverable functionality
very one-way power dynamic with API changes: requires very pro-active engagement with "core" library maintainers to keep up with any potential breaks
can go stale from lack of maintenance if people aren't careful

At the very least I think getting the plugin housed under the inversify organisation would help to alleviate some of that pain, but it still feels like a lot of work for not much benefit (as I see it).

If you want we can have a discussion through discord or any other channel so we can have a videocall and
exchange our different points of view :)

Sounds sensible. Will email you separately.

PodaruDragos · 2024-10-25T08:56:08Z

I would also like to hear the cons here, as far as I can see this enhancement it really does not provide a minus for people who don't want to use this at all. The definitions for ServiceIdentifiers and the autocomplete stays the same so you will get the same experience as if there was no IdentifierMap. I would consider giving this another thought since for the normal inversify user this really does not change a thing. Or maybe I am missing something.

alecgibson · 2024-10-29T17:46:16Z

As discussed offline, the main apprehensions here are that:

Adding a type mapping to this starts to declare a library "philosophy" that might tie the hands of the library later if exploring different philosophies, or trying to stay relatively design agnostic.
@notaphplover 's main concerns right now are simplifying the library (not adding complexity), and internal refactoring
While the maintenance burden of the types should hopefully be low, they're still non-critical, and a break in the type definitions shouldn't block the build of the core library

I still personally would love to see this in the core library one day, but extracting a plugin can hopefully be a low-risk way of battle testing this. I guess weirdly npm stats could also provide a pretty low-effort way of seeing what adoption of the feature is like in terms of downloads vs core library downloads.

In terms of plugin interface, we're leaning towards a pretty minimalist package inside the monorepo, which would just expose type definitions, and consumers can do something like:

import {Container} from 'inversify';
import type {TypedContainer} from '@inversify/strongly-typed'; // (or whatever we call it)
import type {BindingMap} from './binding-map.js';

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

Can do something similar for decorators:

import {inject} from 'inversify';
import type {TypedInject} from '@inversify/strongly-typed';
import type {BindingMap} from './binding-map.js';

export const typedInject: TypedInject<BindingMap> = inject;

(This is basically how we're currently doing it in our own code anyway and seems to work pretty well)

notaphplover · 2024-11-20T23:25:46Z

Closing in favor of the plugin approach in the monorepo

notaphplover · 2024-11-23T13:07:46Z

As promised, a library has been published to provide this feature 🎉. I hope you enjoy it. Special thanks to @alecgibson who made this possible.

alecgibson mentioned this pull request Jul 5, 2024

Type-safe decorators? #788

Closed

alecgibson force-pushed the strongly-typed-injection branch 3 times, most recently from efff40c to 131c0fa Compare July 5, 2024 13:25

alecgibson commented Jul 5, 2024

View reviewed changes

notaphplover reviewed Oct 23, 2024

View reviewed changes

PodaruDragos reviewed Oct 24, 2024

View reviewed changes

review: rename new binding map types

7aa00bb

- `PropertyServiceIdentifier` -> `BindingMapProperty` - `ContainerIdentifier` -> `MappedServiceIdentifier` - revert `ServiceIdentifier` type to previous definition

alecgibson force-pushed the strongly-typed-injection branch from 131c0fa to 7aa00bb Compare October 24, 2024 09:57

alecgibson requested review from PodaruDragos and notaphplover October 24, 2024 09:58

alecgibson mentioned this pull request Oct 31, 2024

feat(strongly-typed): initial commit inversify/monorepo#59

Merged

notaphplover closed this Nov 20, 2024

alecgibson deleted the strongly-typed-injection branch November 25, 2024 09:43

rsaz added enhancement discussion Suggestion labels Dec 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: strongly-typed `Container` #1575

feat: strongly-typed `Container` #1575

alecgibson commented Jul 5, 2024 •

edited

Loading

alecgibson Jul 5, 2024

alecgibson commented Aug 20, 2024

PodaruDragos commented Oct 23, 2024

notaphplover commented Oct 23, 2024

notaphplover left a comment

notaphplover Oct 23, 2024

notaphplover Oct 23, 2024

notaphplover Oct 23, 2024

alecgibson Oct 24, 2024

notaphplover Oct 23, 2024

notaphplover Oct 23, 2024

notaphplover Oct 23, 2024

alecgibson Oct 24, 2024

notaphplover Oct 24, 2024

notaphplover Oct 23, 2024

PodaruDragos Oct 24, 2024

alecgibson Oct 24, 2024

PodaruDragos Oct 24, 2024

alecgibson Oct 24, 2024

PodaruDragos Oct 24, 2024

notaphplover commented Oct 24, 2024 •

edited

Loading

alecgibson commented Oct 25, 2024

PodaruDragos commented Oct 25, 2024

alecgibson commented Oct 29, 2024

notaphplover commented Nov 20, 2024

notaphplover commented Nov 23, 2024 •

edited

Loading

	export type ServiceIdentifier<T = unknown> = (PropertyServiceIdentifier \| Newable<T> \| Abstract<T>);
	export type ServiceIdentifier<T = unknown> = (string \| symbol \| Newable<T> \| Abstract<T>);

	export type ContainerIdentifier<T extends BindingMap> = IfAny<T, ServiceIdentifier, BindingMapKey<T>>;
	export type MappedServiceIdentifier<T extends BindingMap> = IfAny<T, ServiceIdentifier, BindingMapKey<T>>;

		@@ -0,0 +1,124 @@
		import { interfaces } from '../../src/interfaces/interfaces';

feat: strongly-typed Container #1575

feat: strongly-typed Container #1575

Conversation

alecgibson commented Jul 5, 2024 • edited Loading

Description

Motivation

Improved type safety

Non-breaking

Related Issue

How Has This Been Tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

alecgibson commented Aug 20, 2024

PodaruDragos commented Oct 23, 2024

notaphplover commented Oct 23, 2024

notaphplover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notaphplover commented Oct 24, 2024 • edited Loading

alecgibson commented Oct 25, 2024

PodaruDragos commented Oct 25, 2024

alecgibson commented Oct 29, 2024

notaphplover commented Nov 20, 2024

notaphplover commented Nov 23, 2024 • edited Loading

feat: strongly-typed `Container` #1575

feat: strongly-typed `Container` #1575

alecgibson commented Jul 5, 2024 •

edited

Loading

notaphplover commented Oct 24, 2024 •

edited

Loading

notaphplover commented Nov 23, 2024 •

edited

Loading