search

inversify / InversifyJS

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

Unit tests with mocha in watch mode cannot be run multiple times with @injectable decorator #563

Open orendin opened 7 years ago

orendin commented 7 years ago

Hi all I have a problem with my unit tests when I am trying to use Inversify with mocha on node. To be honest, I am not sure if it is an underlying mocha issue or if this could somehow be better handled by either my unit test setup or Inversify.

Expected Behavior

When I run my unit tests using mocha in watch mode mocha --opts ./test/mocha.opts -w, they should always result in the tests passing.

Current Behavior

When I run my unit tests using mocha in watch mode mocha --opts ./test/mocha.opts -w, the first time they work ok, any subsequent time Inversify throws the following exception: Cannot apply @injectable decorator multiple times.

Possible Solution

So I looked into the code in the annotation/injectable.js and spotted this:

 if (Reflect.hasOwnMetadata(METADATA_KEY.PARAM_TYPES, target) === true) {
       throw new Error(ERRORS_MSGS.DUPLICATED_INJECTABLE_DECORATOR);
  }

Which, in turn, let me come up with this fugly work around:

if (!Reflect.hasOwnMetadata("inversify:paramtypes", MyClass)) {
    decorate(injectable(), MyClass);
}

This is obviously far from ideal since it's using magic strings taken out of your library.

Steps to Reproduce (for bugs)

I'll let the code speak in the context section. As described above, I am using mocha in watch mode to continuously run my tests when I change something, which I find a requirement to be productive: mocha -w,

  1. Create a project with code similar to below.
  2. Run mocha in watch mode across the tests.
  3. Watch the tests pass for the first time
  4. Make a change to the class or unit test.
  5. Wait for mocha watch to re-execute the tests
  6. Tests fail with above message.

Context

As suggested in the documentation, I have created a inversify.config.ts file where I configure the container:

import "reflect-metadata";
import { Container } from "inversify";
import TYPES from './types';
import { MyExternalClass } from "external-library";
import { MyClassToTest } from "./myClassToTest";
import { injectable, decorate } from 'inversify';

let container = new Container();
decorate(injectable(), MyExternalClass);

container.bind<MyExternalClass>(TYPES.MyExternalClass).to(MyExternalClass).inSingletonScope();
container.bind<MyClassToTest>(TYPES.MyClassToTest).to(MyClassToTest);

export default container;

Here is a sample version of my test class:

import container from "../inversify.config";
import TYPES from '../types';
import { MyClassToTest } from './myClassToTest';

describe('Test', () => {
    let myClassToTest: MyClassToTest;

    before(() => {
    });

    beforeEach(() => {
        myClassToTest = container.get<MyClassToTest>(TYPES.MyClassToTest);
    });

    it('should work', () => {
        myClassToTest.doSomething();
    });
});

In case it is relevant, I am creating es6 code, and using babel to execute mocha. Mocha options file: ./src/**/*.spec.js --compilers js:babel-core/register --recursive

tsconfig.json:

{
    "compilerOptions": {
        "target": "es6",
        "moduleResolution": "node",
        "module": "commonjs",
        "sourceMap": true,
        "emitDecoratorMetadata": true,
        "lib": [
            "es6",
            "dom"
        ],
        "experimentalDecorators": true,
        "removeComments": false,
        "noImplicitAny": false
    },
    "exclude": [
        "node_modules"
    ]
}

It makes sense to only add the decorator once for the lifetime of the process, which works fine for the actual app. But in the case of unit tests with mocha in watch mode, I have no control over this (at least not that I can see). It seems, the decorator is kept in memory between the executions of the tests. Is there a way in Inversify to officially check if the decorator has been registered before? Or do you know of a way how I can "reset" the state in mocha with watch mode (I couldn't find one)? Or is there something else I can do with my test setup to ensure that the external class is only decorated once (I mean other than my glorious work around)?

Your Environment

Stack trace

[2] Error: Cannot apply @injectable decorator multiple times.
[2]     at d:\repo\someRepo\node_modules\inversify\lib\annotation\injectable.js:8:19
[2]     at DecorateConstructor (d:\repo\someRepo\node_modules\reflect-metadata\Reflect.ts:1090:31)
[2]     at Object.decorate (d:\repo\someRepo\node_modules\reflect-metadata\Reflect.ts:284:20)
[2]     at _decorate (d:\repo\someRepo\node_modules\inversify\lib\annotation\decorator_utils.js:42:13)
[2]     at Object.decorate (d:\repo\someRepo\node_modules\inversify\lib\annotation\decorator_utils.js:52:9)
[2]     at Object.<anonymous> (d:/repo/someRepo/src/inversify.config.js:15:13)
[2]     at Module._compile (module.js:570:32)
[2]     at loader (d:\repo\someRepo\node_modules\babel-core\node_modules\babel-register\lib\node.js:144:5)
[2]     at Object.require.extensions.(anonymous function) [as .js] (d:\repo\someRepo\node_modules\babel-core\node_modules\babel-register\lib\node.js:154:7)
[2]     at Module.load (module.js:487:32)
[2]     at tryModuleLoad (module.js:446:12)
[2]     at Function.Module._load (module.js:438:3)
[2]     at Module.require (module.js:497:17)
[2]     at require (internal/module.js:20:19)
[2]     at Object.<anonymous> (d:/repo/someRepo/src/myClassToTest.spec.js:3:28)
remojansen commented 7 years ago

Hi @orendin how are you importing "reflect-metadata" ? from the command line or form the app?

The problem seems to be caused because the metadata is stored in the Reflect object which is created as a global by reflect-metadata.

My guess is that the mocha watch mode is not cleaning the globals with each run. Can you please check if mocha has some setting force cleaning the globals?

orendin commented 7 years ago

Hey @remojansen, super quick response time, thanks!

It is imported inside the inversify.config.ts at the top (as outlined in the code in the issue, sorry for the long post...).

Hmmm... the globals in mocha. I have already done some research into this and could not find anything useful. But your hint helps to refine the search.

Here we have a 'manual' way of trying to store and reset a global after tests are run in a global hook. https://github.com/mochajs/mocha/wiki/Mess-with-globals

This would mean trying to store a reference to the original Reflect and then resetting it back after the tests ran... I'll give this a shot, thanks!

remojansen commented 7 years ago

No peoblem @orendin, I'm quite sure that that is the problem. If you manage to reset the Reflect global it should work :wink:

orendin commented 7 years ago

I can't seem to get a proper snapshot of the Reflect "Instance" to reset back to.

import "reflect-metadata";
let refl: any = Reflect;

Then in the global after hook, I'm trying to resets the global Reflect back to the original:

after(() => {
    (<any>global).Reflect = refl;
});

But this doesn't do the trick. I don't have time to fiddle around with this any more at the moment and will keep the work-around for now. I'll come back to this later. Thanks!

remojansen commented 7 years ago

No problem, maybe the mocha guys can help.

veeramarni commented 7 years ago

I have the same issue with jest. Wondering if anyone finds a way.

codeandcats commented 5 years ago

I believe I have the same issue with parcel, which watches my React App and performs hot module reloading when source changes. 😞

codeandcats commented 5 years ago

FYI, I've created a PR to address this issue.

The PR exposes a function to disable throwing an error on duplicate injectable decorators. This allows keeping the default functionality while letting devs suffering from this error ignore them.

That said, I don't believe there's actually any benefit from throwing the error at all. Even if the developer has mistakenly applied an injectable decorator twice somehow, there's not really any harm in simply ignoring the "mistake". I don't see a benefit in breaking hard and fast in this particular scenario given it breaks tools which watch like mocha, jest, parcel, etc.

ghost commented 4 years ago

I have the same problem with nextJS in combination with a custom server watched bei nodemon. When i switch pages in nextJS or change anything in my sourcecode, i will receive this error.

Has anyone found a solution to this?

codeandcats commented 4 years ago

I had a PR which fixed the issue but I decided to close it since no one would merge it or give feedback even after I reached out to devs.

codeandcats commented 4 years ago

Side: I’ve given up on using IOC libraries with JavaScript. I’ve found I rarely if ever need to swap out one implementation of some abstraction with another, other than when mocking services in tests, and because Jest makes mocking anything very simple I’ve found it makes IOC libraries for JS largely redundant, for my uses at least.

ghost commented 4 years ago

Thanks for your feedback :) I am developing a large scale business application, so it is kind of a must-have :( I will check out typescript-ioc later today, maybe it will work better in combination with watch modes :)