InversifyJS icon indicating copy to clipboard operation
InversifyJS copied to clipboard

feat: strongly-typed `Container`

Open alecgibson opened this issue 1 year ago • 1 comments

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

https://github.com/inversify/InversifyJS/issues/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)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

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

alecgibson avatar Jul 05 '24 12:07 alecgibson

@PodaruDragos any thoughts on this?

alecgibson avatar Aug 20 '24 12:08 alecgibson

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

PodaruDragos avatar Oct 23 '24 10:10 PodaruDragos

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 avatar Oct 23 '24 10:10 notaphplover

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 :)

notaphplover avatar Oct 24 '24 17:10 notaphplover

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.

alecgibson avatar Oct 25 '24 07:10 alecgibson

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.

PodaruDragos avatar Oct 25 '24 08:10 PodaruDragos

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)

alecgibson avatar Oct 29 '24 17:10 alecgibson

Closing in favor of the plugin approach in the monorepo

notaphplover avatar Nov 20 '24 23:11 notaphplover

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

notaphplover avatar Nov 23 '24 13:11 notaphplover