Controller constructer called twice

[patrickhousley]

• Updated the ActionMetadata callMethod method to use the ControllerMetadata instance getter once.
• Fixes Using inversify, constructor is called twice on controller when instantiated each call #239

[patrickhousley]

Any idea why the tests failed?

[MichalLytek]

@patrickhousley See #243.

[patrickhousley]

Thanks, I can take a look in a bit.

[NoNameProvided]

This PR looks good beside the failing test right? It does for me, since the test isn't failing because of this commit we can merge it.

@19majkel94? Hit the merge if it looks good to you too.

[MichalLytek]

According to the default container code:

const defaultContainer: { get<T>(someClass: { new (...args: any[]): T }|Function): T } = new (class {
    private instances: { type: Function, object: any }[] = [];
    get<T>(someClass: { new (...args: any[]): T }): T {
        let instance = this.instances.find(instance => instance.type === someClass);
        if (!instance) {
            instance = { type: someClass, object: new someClass() };
            this.instances.push(instance);
        }

        return instance.object;
    }
})();

The instance is created only once, the second call for this.controllerMetadata.instance returns the cached instance from array. So what is the problem with inversify? If it always create instance on container.get(), still every request would create new instance of the controller.

The changes is not dangerous but I don't know what it adds. @patrickhousley @appsolutegeek can you explain this? 😉

[NoNameProvided]

The code you linked is from typeDI right? I think this fixes that inversify will always return a new instance instead of a cached one? So by saving it first into a variable and then using that will prevent getting it twice from the container thus preventing the double constructor call.

[MichalLytek]

The code you linked is from typeDI right?

No, it's from the default container:
https://github.com/pleerock/routing-controllers/blob/master/src/container.ts

TypeDI works in the same way, but it handle more complicated stuffs like dependencies:
https://github.com/pleerock/typedi/blob/master/src/Container.ts

[patrickhousley]

@19majkel94 The reason the default container and TypeDI work is because they cache the instance of the class constructed (like you said). In Inversify terms, that is singleton scope. Inversify, by default, actually uses transient scope meaning every time a resource is requested, a new instance is created and returned. (docs)

[MichalLytek]

So why don't config inversify to singleton scope? What is the purpose to have new instance of controller on each request?

[iangregsondev]

This is actually a good question. Why not set to singleton. It certainly works and only calls the constructor once but this means the controller lives for the length of the app. Is this really good practice?

Also because the controller is injected with services and repositories and these are not singleton. They act like singletons because the controller constructor is only called once so it's never ever injected again.

[patrickhousley]

I have been trying to find concrete information on this subject for Node but have not been very successful. Needless to say, if this was Java or .Net, I would confidently say that we should use transient scope due to thread safety....yada yada yada. For Node, there are no "threads" but there is the event loop. @appsolutegeek brings up a good point, if the controllers are singleton then any of their transient dependencies will appear to be singleton. I have also talked to a fellow senior developer and he recommended using singleton scope any time state is being accessed (req/res objects due count as state). I really wish there were more info and more studies done on the subject but I would at least err on the side of caution and use transient scope for the controllers.

[MichalLytek]

Node.js is single threaded, event-loop based, and it is its biggest advantage. We won't waste resources for creating separate threads and new instances of every controller and services. All the data related to the request (state) should be attached to the req/res/ctx objects. It's a common way to do this in conventional express (or koa) based apps to pass data between middlewares and to the route handler. And it really work in node.js ecosystem as it's designed in this way, along with the async paradigm.

Controller method (route action) as well as services should be stateless, so request doesn't interfere each other. They don't have to be pure functions with no side-effects, the can call an API or the db of course, but should only rely on their arguments, not the app state, global/static variables, etc.

The only use case that has been registered is the slightly reducing boilerplate for things like logging service which might need direct acccess to the request object (#174).

[MichalLytek]

Small change but might help someone 😉

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.