inversify / InversifyJS

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

Feature request: Add forgiving version of decorate() #1042

Open bodograumann opened 5 years ago

bodograumann commented 5 years ago

When using third party classes as base-classes for injectable classes, they need to be decorated with decorate(injectable(), BaseClass). This must happen exactly once. So when multiple independent modules extend BaseClass, it is not clear which of them should call decorate.

Consider the following example:

          BaseClass
           ╱     ╲
          ╱       ╲
@injectable()    @injectable()
ModuleA          ModuleB
          ╲       ╱
       ╲     ╱
       CompositionRoot

The composition root does not know, the the modules are using the undecorated base class and the two modules don’t know anything about each other.

Furthermore both modules could possibly be used on their own without the other being present. Thus both should decorate the base class with the injectable annotation.

Workaround

Currently I am calling

try {
  decorate(injectable(), BaseClass)
} catch {}

This works, but the downside is, that all other errors during the decoration are also ignored.

Possible Solution

I can think of two possible solutions:

Personally I would prefer the first one, but maybe there are good reasons to not ignore this error, which I currently can not think of⁉

tonyhallett commented 5 years ago

Solution 1 - Use the container option skipBaseClassChecks and then no need for the injectable decorator on the base class. Although if you do this you will lose some parameter count checking which you may be relying upon - see inheritance.

Solution 2 - replicate the injectable decorator without the exception :

it('should be ok to decorate twice with safeinjectable',()=>{
            function safeinjectable() {
                return function(target: any) {

                  if (Reflect.hasOwnMetadata(METADATA_KEY.PARAM_TYPES, target)) {

                  }else{
                    const types = Reflect.getMetadata(METADATA_KEY.DESIGN_PARAM_TYPES, target) || [];
                    Reflect.defineMetadata(METADATA_KEY.PARAM_TYPES, types, target);
                  }

                  return target;
                };
              }
            class Inject{

            }
            class SafeInject{}

            decorate(injectable(),Inject)

            expect(()=>decorate(injectable(),Inject)).toThrow();

            decorate(safeinjectable(),SafeInject);
            expect(()=>decorate(safeinjectable(),SafeInject)).not.toThrow();

        })

Solution 3 - safeInjectable to try/catch on injectable - which is probably a better choice than 2 as there is no knowledge of inversify internals.

Personally I agree that decorate should be forgiving.