inversify / InversifyJS

A powerful and lightweight inversion of control container for JavaScript & Node.js apps powered by TypeScript.
http://inversify.io/
MIT License
11.34k stars 719 forks source link

Expose container binding dictionary #1584

Closed c100k closed 1 month ago

c100k commented 1 month ago

For multiple reasons, it can be useful to access a container's bindings : write some tests, check that the bindings are correct, export the bindings to a human readable format, etc.

Possibly related topic : https://github.com/inversify/InversifyJS/issues/1410.

Expected Behavior

The Container has a _bindingDictionary field (https://github.com/inversify/InversifyJS/blob/master/src/container/container.ts#L25). Since the field is private, we could add a public getter for it.

Current Behavior

Right now, there is no getter so the only workaround is to access the field telling TypeScript to stop complaining with @ts-ignore.

console.log(container._bindingDictionary);

Even if it "works", it's hacky and not very ideal. Because of @ts-ignore, if for any reason inversify is updated and this field is renamed/removed, tsc won't complain and it will cause an error at runtime.

Possible Solution

Adding a getter to access the bindings map.

Steps to Reproduce (for bugs)

N/A

Context

N/A

Your Environment

Stack trace

notaphplover commented 1 month ago

Hey @c100k, as said in #1410, I am afraid the container is not so simple. The binding dictionary is private for some very good reasons:

I don't think we should expose this dictionary for now. We might extend the container API with a getAllBindngs method instead to provide what you guys expect from _bindingDictionary.

c100k commented 1 month ago

Hi @notaphplover,

Thanks for your explanation. Indeed, it makes a lot of sense.

The use cases described in this issue are "read-only", so a getAllBindings method would do the job perfectly. I guess it should rely on something like this._bindingDictionary.traverse as used in unbindAll ?

Happy to help, develop, test or simply debug if need be.

notaphplover commented 1 month ago

Hey @c100k

Happy to help, develop, test or simply debug if need be.

To be honest I need as much help as possible, I want to continue maintaining the library after a couple of years not being properly maintained.

If you are interested in submiting a PR, allow me to give you my 5 cents:

Implementation notes

Introduction

What to return

We should probably return the result of merging clones of the container _bindingDictionary starting with the parent.

How would we merge a Lookup?

Given this and lookup Lookups, merging lookup into this is defined as follows:

This way merging a child in a parent overrides services the same way we want. Keep in mind this operation is asociative (merge(merge(a, b), c) is the same as merge(a, merge(b, c))).

This would be implemented in a merge method in the Lookup class.

How do we implement getAllBindings?

Given this container. Given _getAllBindings a function that accepts a this Container and a lookup Lookup.

_getAllBindings implementation

_getAllBindings is defined as follows:

Note: this implementation makes way more clones than necessary, we can probably improve this to only make a clone of the highest ancestor's _bindingDictionary.

getAllBindings implementation

Simply return this._getAllBindings(this._bindingDictionary.clone()).

What do you think about it? I hope I made myself clear, otherwise feel free to ask for any doubts in your mind.

rsaz commented 1 month ago

I actually would like to see a real good use case on why you would like to access what is inside of the _bindingDictionary.

If your reason is testing, you're actually violating the basic idea of the library which is to provide you a consistent, reliable and trustworthy ecosystem in which you can implement your functionalities or system on top of the container mechanic.

The reason why the _bindingDictionary is private in the Container class is not just to encapsulate the data but to protect the internal state of the container. Probably this was the goal of the author, this design choice prevents external code from directly modifying the _bindingDictionary, to ensure that bindings are managed in a controlled and predictable manner through the provided public methods.

Now, if the use case is different resolution strategies, we can discuss those, are those strategies being aimed for. Or maybe you want to design a library that integrates well with the container but for this you need access to the bindings, or maybe the application itself can adjust its own behavior based on a specific configuration of certain bindings.

Look, all these ideas above are just hypotheses of potential implementations, they don't reflect what this ioC should have at the moment. I was just comparing Inversify with Microsoft iOc (https://github.com/microsoft/tsyringe) is very minimalist and doesn't have a bunch of things Inversify already have. I think for this one, we just need to find a real good use case to have this exposed. I honestly have mutability, security and performance concerns, without having a proper use case to study. (Basically, what are you trying to achieve)

A suggestion of implementation would a function like this, if you pass the serviceIdentifier it will return the bindings for you, otherwise if nothing is passed, it will return all bindings.

public listBindings(serviceIdentifier?: interfaces.ServiceIdentifier<unknown>): ReadonlyArray<{ serviceIdentifier: interfaces.ServiceIdentifier<unknown>; bindings: ReadonlyArray<interfaces.Binding<unknown>> }> {
    const bindingsList: Array<{ serviceIdentifier: interfaces.ServiceIdentifier<unknown>; bindings: ReadonlyArray<interfaces.Binding<unknown>> }> = [];

    this._bindingDictionary.traverse((key, value) => {
        if (!serviceIdentifier || key === serviceIdentifier) {
            bindingsList.push({ serviceIdentifier: key, bindings: Object.freeze([...value]) });
        }
    });

    return Object.freeze(bindingsList);
}

Example of return:

@injectable()
class A {
    public name: string;
    public age: number;
}

@injectable()
class B {
    public name: string;
    public age: number;
}

@injectable()
class User {
    public name: string;
    public age: number;
}

const container = new Container();
container.bind(User).toSelf();
container.bind(A).toSelf();
container.bind(B).to(User);
container.bind(User).to(B);
console.log(container.getBindingDictionary(B));

Return

{
  'class B {\n    name;\n    age;\n}': [
    Binding {
      id: 3,
      activated: false,
      serviceIdentifier: [class B],
      scope: 'Transient',
      type: 'Instance',
      constraint: [Function (anonymous)],
      implementationType: [class User],
      cache: null,
      factory: null,
      provider: null,
      onActivation: null,
      onDeactivation: null,
      dynamicValue: null
    }
  ]
}

Tell me if make sense to you, then we can close this issue until you find a good real use case in which we can study and provide you a good solution. What are your thoughts?

notaphplover commented 1 month ago

Hey @rsaz, thank you for giving your perspective.

I actually would like to see a real good use case on why you would like to access what is inside of the _bindingDictionary.

If your reason is testing, you're actually violating the basic idea of the library which is to provide you a consistent, reliable and trustworthy ecosystem in which you can implement your functionalities or system on top of the container mechanic.

To be really honest, agree. Inversify has way too many features to be maintained, and we should have a really good use case in order to provide this feature.

@c100k could you give us a minimum example in which you would find this feature useful?

Look, all these ideas above are just hypotheses of potential implementations, they don't reflect what this ioC should have at the moment. I was just comparing Inversify with Microsoft iOc (https://github.com/microsoft/tsyringe) is very minimalist and doesn't have a bunch of things Inversify already have. I think for this one, we just need to find a real good use case to have this exposed. I honestly have mutability, security and performance concerns, without having a proper use case to study. (Basically, what are you trying to achieve)

Yeah, performance might be a concern here. I would say mutability and security issues can be easily solved using clones.

A suggestion of implementation would a function like this, if you pass the serviceIdentifier it will return the bindings for you, otherwise if nothing is passed, it will return all bindings.

I feel returning container bindings feels incomplete with the existence of parent containers.

c100k commented 1 month ago

Hi @rsaz,

Thanks for your insights. Here is a typical use case I'm facing:

I'm building a library/framework on top of Inversify allowing developers to build business applications faster (not released yet). By default, the library comes with a preconfigured container, with default bindings. Optionally, the user can rebind what they want. And to give them a full overview of what is bound, I'm traversing the bindingDictionary to show what is bound to what in a report. It looks like this:

CryptoManager                      : CryptoManager                      <-- NodeCryptoManager [type: Instance] [scope: Transient]
DateTimeManager                    : DateTimeManager                    <-- LuxonDateTimeManager [type: Instance] [scope: Transient]
EmailManager                       : EmailManager                       <-- FakeEmailManager [type: Instance] [scope: Transient]
EnvironmentManager                 : EnvironmentManager                 <-- NodeEnvironmentManager [type: Instance] [scope: Transient]
FormDataBuilder                    : FormDataBuilder                    <-- NodeFormDataBuilder [type: Instance] [scope: Transient]
FSManager                          : FSManager                          <-- NodeFSManager [type: Instance] [scope: Transient]

Note that I'm not facing any issues with any parent containers (as mentioned by @notaphplover) since I only have one container, to keep things simple for now.

The library/framework also comes with auto-testing features so the idea is to check if everything is bound correctly. For example, we would alert the developer if for a production environment, they kept FakeEmailManager as implementation and so on.

I agree that for this last use case, I can check dependencies one by one with the serviceIdentifier. I also agree that this use case might be an edge case the library does not want to support. If we don't proceed with it, I'll keep the workaround for the beta and figure out maybe a better solution along the way. After all, development is always about trade-offs :-).

A last use case I see maybe, is to detect forgotten bindings statically instead of at runtime. Indeed, with the right reflection usage, one can check what is injected in their classes and compare it with what is bound in the container. And on the other hand, it could also detect obsolete bindings.

PS : @notaphplover thanks for suggesting how this would be implemented. Even though we don't more forward, it's a great learning to understand how the library works.

rsaz commented 1 month ago

Hi @c100k thanks for your reply.

I am happy that you're creating a lib/framework with Inversify. Keep us informed so that we can use your tool in the future and provide feedback as well. I built ExpressoTS Framework (https://expresso-ts.com/), the third most downloaded framework for building backend API's, just behind Koa and Nestjs. ExpressoTS is powered by Inversify as well, that's why I decided to support the project.

Now we do have two different templates when opening issues, one for "Fixing Bugs" and the other for "Community Ideas" in which this request would fall into.

image

When you're ready, with a good use case for this request, open a community idea (feature request), and we go from there to generate a proper documentation to start implementing this feature.

Your idea is not bad at all, as I said, it just needs to have a strong argument to be build, especially in this phase 1, where me and @notaphplover are working to reorg the project and make sure is clean and set to success.

I am closing this ticket if you don't mind!