lukeautry / tsoa

Build OpenAPI-compliant REST APIs using TypeScript and Node
MIT License
3.45k stars 496 forks source link

ControllerFactory / IocContainerDactory as `RegisterRoutes` argument. #855

Open fahrradflucht opened 3 years ago

fahrradflucht commented 3 years ago

Sorting

Expected Behavior

I would like to instantiate the controllers myself without the need to create an iocModule.

The reason for that is, that I'm not using some fancy ioc framework, but instead just want to orchestrate my application boot order (connect to the database, load config, etc) by hand and then dependency inject some objects myself by passing them as constructor arguments to the controllers. Plain and simple manual dependency injection.

One could do that by using the already present iocModule but it would involve some hacky global module state, and become very inconspicuous.

The best way to support this, in my opinion is to allow passing controller(-factories) to RegisterRoutes.

Current Behavior

It is not possible to instantiate the controllers in user land, outside of using the iocModule feature or a custom routes template.

Possible Solution

I solved this for my project by using a custom routes template that, expressed as a diff against express.hbs looks like this:

diff --git a/packages/cli/src/routeGeneration/templates/express.hbs b/packages/cli/src/routeGeneration/templates/express.hbs
index 7bc5e0c..1bcc113 100644
--- a/packages/cli/src/routeGeneration/templates/express.hbs
+++ b/packages/cli/src/routeGeneration/templates/express.hbs
@@ -1,7 +1,7 @@
 /* tslint:disable */
 /* eslint-disable */
 // WARNING: This file was auto-generated with tsoa. Please do not modify it. Re-run tsoa to re-generate this file: https://github.com/lukeautry/tsoa
-import { Controller, ValidationService, FieldErrors, ValidateError, TsoaRoute, HttpStatusCodeLiteral, TsoaResponse } from '@tsoa/runtime';
+import { Controller, ValidationService, FieldErrors, ValidateError, TsoaRoute, HttpStatusCodeLiteral, TsoaResponse, IocContainer, IocContainerFactory } from '@tsoa/runtime';
 {{#each controllers}}
 // WARNING: This file was auto-generated with tsoa. Please do not modify it. Re-run tsoa to re-generate this file: https://github.com/lukeautry/tsoa
 import { {{name}} } from '{{modulePath}}';
@@ -11,7 +11,6 @@ import { expressAuthentication } from '{{authenticationModule}}';
 {{/if}}
 {{#if iocModule}}
 import { iocContainer } from '{{iocModule}}';
-import { IocContainer, IocContainerFactory } from '@tsoa/runtime';
 {{/if}}
 import * as express from 'express';

@@ -45,7 +44,10 @@ const validationService = new ValidationService(models);

 // WARNING: This file was auto-generated with tsoa. Please do not modify it. Re-run tsoa to re-generate this file: https://github.com/lukeautry/tsoa

-export function RegisterRoutes(app: express.Router) {
+export function RegisterRoutes(
+    app: express.Router,
+    iocContainerFactory?: IocContainerFactory
+) {
     // ###########################################################################################################
     //  NOTE: If you do not see routes for all of your controllers in this file, then you might not have informed tsoa of where to look
     //      Please look into the "controllerPathGlobs" config option described in the readme: https://github.com/lukeautry/tsoa
@@ -80,7 +82,17 @@ export function RegisterRoutes(app: express.Router) {
                 controller.setStatus(undefined);
             }
             {{else}}
-            const controller = new {{../name}}();
+            let controller;
+            if (iocContainerFactory) {
+              const container: IocContainer = iocContainerFactory(request)
+
+              controller = container.get<{{../name}}>({{../name}});
+              if (typeof controller['setStatus'] === 'function') {
+                  controller.setStatus(undefined);
+              }
+            } else {
+              controller = new {{../name}}();
+            }
             {{/if}}

This is not necessarily the way to do official support (although maybe it is), but it was the one I deemed to be the least intrusive currently.

What do you think of this addition? What you accept a PR that would introduce something along this lines? Obviously it would need tests, and I also haven't thought about hapi or Koa yet.

Steps to Reproduce

Not applicable.

Context (Environment)

Version of the library: 3.4.0 Version of NodeJS: 14.15.1

Breaking change?

No, this wouldn't be a breaking change.

WoH commented 3 years ago

I"m not sure I understand your point as a chain of implications and not a conflation of 2 issues, so let's see about what I understand to be 2 separate issues.

1) I understand you want want to invert the control and create the Controllers yourself. That's perfectly reasonable, and you can in fact do that by exporting anything that confirms to the interface documented here.

I'm not sure if you were aware of that or not, as you continued the discussion with:

One could do that by using the already present iocModule but it would involve some hacky global module state, and become very inconspicuous. The best way to support this, in my opinion is to allow passing controller(-factories) to RegisterRoutes.

If you were aware of that, the argument would be:

2) I don't want to export an object from a module, but instead add it as an argument to RegisterRoutes. This would obviously only make sense if we could pass both a container or a factory (that implement the interface linked above. Does that mean all the prologue is actually not relevant to the issue raised?

In that case the argument for not using the current way is: "hacky global module state": I had some text here seriously trying to discuss this, but it's either too vague to tell, wrong or the alternative would not be different. Inconspicuous is subjective, a factory.ts next to the index.ts is the most conspicuous way I could imagine.

E: I'd even agree we should consider adding it as an argument, but the latter part of that issue is not it.

tsimbalar commented 3 years ago

For what it's worth, we are currently using tsoa without any magic IoC. Just in case it is useful, here is how we do it :

{
    "entryFile": "src/start.ts",
   // ...
    "routes": {
        "routesDir": "src/api",
        "routesFileName": "tsoa-routes.generated.ts",
        "iocModule": "src/api/tsoa-ioc",
        "authenticationModule": "src/api/tsoa-auth"
    },
    "ignore": [
        "**/src/!(api)/**/*.ts"
    ]
}
// needed for tsoa to know how to create controller instances

import { IControllerFactory } from '../infrastructure/bootstrap/IControllerFactory';
import { IocContainer } from '@tsoa/runtime';

class IocContainerAdapter implements IocContainer {
  public constructor(private readonly controllerFactory: IControllerFactory) {}

  public get<T>(controller: { prototype: T }): T {
    return this.controllerFactory.get<T>(controller as any);
  }
}

export function RegisterControllerFactory(controllerFactory: IControllerFactory): void {
  iocContainer = new IocContainerAdapter(controllerFactory);
}

// tsoa wants an export named `iocContainer`
export let iocContainer: IocContainer;
export type ConstructorFunction<T> = new (...args: any[]) => T;

export interface IControllerFactory {
  get<T>(controllerConstructor: ConstructorFunction<T>): T;
}

i.e. given a Controller type (provided as a constructor function), give me an instance of it.

import express from 'express';
import { RegisterRoutes } from './tsoa-routes.generated';
import { RegisterControllerFactory } from './tsoa-ioc';

export function buildWebApp(
  compositionRoot: CompositionRoot, // <- something that implements IControllerFactory
): express.Express {
  const app: express.Express = express();

  // snip ... register logging middleware and general express stuff like cors and helmet

  // -- TSOA hook --
  RegisterControllerFactory(compositionRoot); // <- this is from our tsoa.ioc.ts
  RegisterRoutes(app);

  // -- ERROR HANDLING --
  // (must be at the end)
  // more express middleware ... 

  return app;
}

I hope this helps

fahrradflucht commented 3 years ago

I don't want to export an object from a module, but instead add it as an argument to RegisterRoutes. This would obviously only make sense if we could pass both a container or a factory (that implement the interface linked above. Does that mean all the prologue is actually not relevant to the issue raised?

I thought if I just say "I want to pass controller factories to RegisterRoutes." you would naturally ask "why?" that's why I gave context of what I'm actually trying to achieve. I gave the context because I think if you are already using an IoC framework the current way of using a module is actually okayish - its just that if you want to write more plain and simple code that the indirection feels a little awkward.

E: I'd even agree we should consider adding it as an argument, but the latter part of that issue is not it.

Then I guess I don't have a case and I would be happy to hear your better arguments 😅. - Seriously though, what I meant with "hacky global module state" is basically exactly what @tsimbalar exemplified. Don't get me wrong it's an okay approach but to me, it feels indirect and when you read that server.ts example it's kind of obvious that what you actually want is pass the compositionRoot to the RegisterRoutes call and are just forced to do this workaround. Now suddenly you have introduced global module state, which makes your buildWebApp function have side effects that can cause issues when you use it in tests, especially concurrent ones.

I hope I'm making more sense to you this time.

tsimbalar commented 3 years ago

@fahrradflucht agreed that we are jumping through more hoops than needed here ... and yes, there is global state / temporal coupling with the approach we took, and it is indeed somehow a workaround (FYI we ended up with this from a migration from typescript-rest to tsoa) . FYI we have something in the same vein for setting up Authentication.

I think I know see your point about passing things to RegisterRoutes.

I think maybe it could be a non breaking change to turn the generated

export function RegisterRoutes(app: express.Router) {

into

export function RegisterRoutes(app: express.Router, containerFactory? : IocContainerFactory) {

(not in love with the names though ... maybe it could just forget about existing IocContainer namings and just be a

// given "type information" (i.e. a constructor for a Controller) , create an instance of it for the current request
export declare type ControllerFactory = (controller: {
        prototype: Controller;
    }, request: unknown) => Controller;

export function RegisterRoutes(app: express.Router, controllerFactory? : ControllerFactory) {

)

Then in the generated routes file we could check if controllerFactory is provided , and use it to construct controllers when needed. Possibly we'd have to reject cases where there is both a iocModule (from tsoa.json) and a controllerFactory as they would be ambiguous etc.

Then this could actually be extended to add support for authentication as well if we want to do it the same way.

We could even go as far as :

// given "type information" (i.e. a constructor for a Controller) , create an instance of it for the current request
export declare type ControllerFactory<TRequest> = (controller: {
        prototype: Controller;
    }, request: TRequest) => Controller;

export declare type TsoaAuthenticationHandler<TRequest> = (
  request: TRequest, // <- the concrete type of Request depending on the framework
  securityName: string,
  scopes?: string[],
) => Promise<any>

interface RegisterRoutesOptions<TRequest> = {
  controllerFactory?: ControllerFactory<TRequest>,
  authenticationHandler?: TsoaAuthenticationHandler<TRequest>
}

export function RegisterRoutes(app: express.Router, options? : RegisterRoutesOptions) {

meaning passing things as modules through tsoa.json is just one way of doing things, but we can also do it in a more direct way by passing parameters to RegisterRoutes. This makes it also easier and more explicit for newcomers to a codebase, as they don't need to understand the extra indirection between tsoa.json , and a file that ends up being magically loaded.

Just my 2 cents :)