Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Confusing issue with resolving class if module is called from separate module #262

Closed
tenowg opened this issue Jun 21, 2016 · 10 comments
Closed
Assignees
Labels
Milestone

Comments

@tenowg
Copy link
Member

tenowg commented Jun 21, 2016

Maybe you can help me resolve this issue. This is likely behavior that I didn't account for in my code.

var k: i.Kernel = new Kernel();
export var pInject = makePropertyInjectDecorator(k);

k.bind<DefaultOptions>(DefaultOptions).toConstantValue(new DefaultOptions());
k.bind<RouteBuilder>(RouteBuilder).to(RouteBuilder).inSingletonScope().onActivation((context, router) => {
    router.kernelInstance = k;
    return router;
});
k.bind<ExpressMvc>(ExpressMvc).to(ExpressMvc).onActivation((context, expressify) => {
    expressify.kernel = k;
    return expressify;
});

/**
 * The main function called to create a ExpressMvc object, initialized the DI and returns a useable ExpressMvc object
 *
 * @param {...mvcController} ...controllers The list of controllers to add to DI, all controllers used are required.
 * @return {ExpressMvc}
 */
export function expressMvc(...controllers: any[]): ExpressMvc {
    debug("Starting ExpressMVC");
    // Handle registering Controllers
    controllers.forEach(controller => {
        debug(`Binding controller (${controller.name})`);
        k.bind<mvcController>(controller).to(controller);
        debug(`Bound controller (${controller.name})`);
    });
    debug('Finished binding controllers...');
    return k.get<ExpressMvc>(ExpressMvc);
}

export function getRouter(): RouteBuilder {
    return k.get<RouteBuilder>(RouteBuilder);
}

When this code is called from identical code from within the module, it works without an issue, when I seperate the test code (just a simple module that is supposed to run the server) the getRouter() function will error with `Missing required @Injectable annotation in: RouteBuilder.

The getRouter() is used in functions as such:

/**
 * route is optional, if not supplied will use a slug-case case of the function name as route
 */
export function HttpGet(options?: {
        route?: string,
        parameters?: string
    }) {
    return (target: any, propertyKey: string, descriptor: TypedPropertyDescriptor<any>) => {
        if (!options || !options.route) {
            options = options || {};
            options.route = "/" + require('to-slug-case')(propertyKey);
        }

        getRouter().registerHandler(AllowedMethods.GET, options.route, target, propertyKey, options.parameters || "");     
    }
}

which are annotations on methods in controller classes.

The main code that is run to start the application is:

import {expressMvc} from "@demgel/mvc";
import {TestController} from "./controllers/testController";
import {TestService} from "./services/testService";

let server = expressMvc(TestController);
server.addTransient<TestService>(TestService, TestService);
server.setViewEngine("../../views", 'pug');
// server.setFavicon('');
server.listen(3000);

If this is in the @demgel/mvc module it will work When I separate this code to a new module, I get the error.

additionaly, if I remove any controllers from the startup, RouteBuilder is injected into ExpressMvc when it is resolved at the end of the expressMvc function.

@remojansen
Copy link
Member

When you say a module are you talking about an ES6 module or an npm module? Does RouteBuilder extend a class? Can I find the full source code on github? Thanks

@remojansen
Copy link
Member

remojansen commented Jun 21, 2016

I'm not fully sure but my guess is the following...

I'm assuming that you have 2 npm modules.

The @injectable() annotation uses Reflect.defineMetadata under the hood. The Reflect object is mean to be a global.

If you are using multiple npm modules maybe what is happening is that you have 2 instances of Reflect (one per module):

  • In one module you create some metadata using Reflect.defineMetadata via @injectable()
  • In the second module, you try to read the metadata using Reflect.getMetadata via kernel.get().

The problem is that if there are two Reflect objects the second will fail. I have never encounter this problem so it would be great if you could debug and find it.

Invoking the following:

Reflect.getMetadata("inversify:paramtypes", RouteBuilder);

Should return an empty array in both npm modules. The solution to this problem would be to ensure that Reflect is accessed as a singleton global.

@tenowg
Copy link
Member Author

tenowg commented Jun 21, 2016

On 06/21/2016 06:40 PM, Remo H. Jansen wrote:

I'm not fully sure but my guess is the following...

I'm assuming that you have 2 npm modules.

The |@Injectable()| annotation uses |Reflect.defineMetadata| under the
hood. The Reflect object is mean to be global. If you are using
multiple npm modules maybe what is happening is that you have to
instances of Reflect (one per module):

  • In one module you read create some metadata using
    |Reflect.defineMetadata| via |@Injectable()|
  • In the second module, you try to read the metadata
    |Reflect.getMetadata| via |kernel.get()|. The problem is that if
    there are two Reflect objects the second will fail.

I have never encounter this problem so it would be great if you could
debug and find it:

Reflect.getMetadata("inversify:paramtypes", RouteBuilder);

Should return an empty array in both npm modules.

You seem to be on the right track... if I call Reflect... from test.js
(the test module setupfile) it return an emtpy [], but from getRouter()
it returns undefined... any ideas on how to fix this?

@remojansen
Copy link
Member

remojansen commented Jun 21, 2016

When it returns undefined is because a new instance of Reflect has been imported and it doesn't contain the metadata from the first instance.

If you are going to distribute the npm modules as librarires then they shouldn't import reflect-metadata,

You need to remove the following from your code:

import "reflect-metadata";

This line should be be used only once by the consumers.

The consumers are the ones that need to import it. For example, InversifyJS uses Reflect but you as a consumer need to import it.

We do import reflect when we run the unit test because the unit test can be considered as a consumer.

@tenowg
Copy link
Member Author

tenowg commented Jun 22, 2016

On 06/21/2016 07:37 PM, Remo H. Jansen wrote:

If you are going to distribute the npm modules as librarires then they
shouldn't import |reflect-metadata|,

You need to remove the following from your code:

import "reflect-metadata";

The consumers are the ones that need to import it. For example,
InversifyJS uses Reflect but you as a consumer need to import it.

We do import reflect when we run the unit test
https://github.com/inversify/InversifyJS/blob/master/gulpfile.js#L174 because
the unit test can be considered as a consumer.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#262 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABGCIv_SY0oe1K8-JrurHLapm_umg9nYks5qOHXTgaJpZM4I7JbS.

Thanks, I had thought of the same thing, but was missing an import from
my validator module that was causing the issue... thanks for all the
help, not sure if I would have looked for it had not for you confirmation

@remojansen remojansen added this to the 2.0.0-rc.1 milestone Jun 22, 2016
@remojansen
Copy link
Member

No problem. Can this issue be closed then?

@remojansen
Copy link
Member

@tenowg can you please confirm if this issue be closed? Thanks!

@remojansen remojansen self-assigned this Jun 23, 2016
@remojansen remojansen mentioned this issue Jun 23, 2016
18 tasks
@tenowg
Copy link
Member Author

tenowg commented Jun 23, 2016

Yes, this issue is closed... thanks again

@voxmatt
Copy link

voxmatt commented Oct 17, 2016

I know this is a closed issue, but @remojansen and @tenowg thank you for working through this in public — just saved me from pulling out the last of my hair!

@remojansen
Copy link
Member

No problem :) Feel free to create issues instead of using the chat if you have any questions. Issues can help other users and at the moment we don't get a crazy number of issues. If it scales to too many issues per day we will change the rules but for the moment is fine to create issues 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants