Proposal: New loading controllers/interceptors/middlewares API

[MichalLytek]

Ref. https://github.com/pleerock/routing-controllers/issues/202#issuecomment-321031119

Introduction

Right now, RoutingControllersOptions has following signatures for controllers/interceptors/middlewares - Function[] | string[] - which allows to register in the framework list of classes or directories from where to import.

The array syntax is great because it let's you define the order of middlewares explicitly:

createExpressServer({
    middlewares: [
        MorganMiddleware,
        JwtMiddleware, 
        AuthMiddleware, 
        CompressionMiddleware, 
        LoggingMiddleware, 
        ErrorMiddleware,
    ],
});

So there's no need to jump through middlewares files and looking for priority option.

However, in big apps it might be not comfortable to list all of 58 controllers explicitly in array, so we have support for glob string and loading from directories:

createExpressServer({
    controllers: [__dirname + "/controllers/**/*.js"],
})

The cons are that you can't disable one middleware for dev/debug purpose and you have to use priority option in middleware decorator to declare the order of calling them which is very hard in maintaining.

In TypeScript and ES6 we can export and import from modules, so it's a common case to have index files. The directory structure looks like this:

middlewares
- auth-middleware.ts
- index.ts
- jwt-middleware.ts
- logging-middleware.ts
// etc.

And the index.ts file looks like this:

export * from "./jwt-middleware";
export * from "./auth-middleware";
export * from "./logging-middleware";

And in app.ts:

import * as middlewares from "./middlewares";

Because all object properties are traversed "in the order in which they were added to the object", we can use index.ts to manipulate the explicit order of middlewares. It doesn't have to be placed as index file, it might be placed in app configuration folder with modified paths.

Main proposal

Right now we can't pass object to routing-controllers option. So we need dirty hacks like:

createExpressServer(
    middlewares: Object.keys(middlewares).map(key => (<any>middlewares)[key]),
});

I propose to add support for objects containing middlewares/controllers/interceptors. We could then just do:

import * as middlewares from "./middlewares";
createExpressServer(
    middlewares: middlewares,
});

or with ES6 shorthand syntax:

import * as middlewares from "./middlewares";
import * as controllers from "./controllers";
import * as interceptors from "./interceptors";

createExpressServer({
    middlewares,
    controllers,
    interceptors,
});

Also I think that importing all from directories by glob string is an antipattern and this proposal reduce a lot of boilerplate of explicit array option. If users get used to this feature I would even deprecate loading from directories feature.

Adding @NoNameProvided @pleerock for discussion.

[NoNameProvided]

As we discussed this already and agreed we like this approach, but just to have it here, +1 👍 for me on this idea.

I guess we are waiting only for @pleerock to give his blessing on the idea.

[pleerock]

Also I think that importing all from directories by glob string is an antipattern

loading entities / controllers / anything else from directories is efficient, productive and pragmatic solution for developers. So I would recommend to use it for everyone who have big apps.

In my opinion having a single index.ts file which kinda exports everything from a single directory is a bad solution simply because its additional maintenance pain. Its easy to forget to add newly added controller into this file and then think why controller is not working. Also you need to add and remove controllers each time (so you need to maintain index.ts file) which means extra boilerplate work. Also you'll have extra "index.ts" file inside your folder structure which does not look good:

- BlogController.ts
- CategoryController.ts
- index.ts
- UserController.ts

I'm not against this feature if you both need it, but you heard my opinion on it.

[MichalLytek]

it's efficient, productive and pragmatic solution for developers. So I would recommend to use it for everyone who have big apps.

If you would have to refactor the enormous app with 30 custom middlewares, 50 interceptors and 200 controllers then yes, it might be waste of time. But during debugging/developing you have never needed to disable some middleware/interceptor? It's awful with glob string imports as you need to comment the code of the class. And the biggest problem in huge apps is the middlewares priority problem as you can't just see the explicit order line by line but only jumping through 30 files and debugging @Middleware decorator priority option in your head. So yes, this is efficient until you have to figure out why something in your app is not working 😜

you'll have extra "index.ts" file inside your folder structure which does not look good

I always do index files as it's great in combination with tsconfig.json "Path mapping" feature and tsconfig-paths module - you can have nice readable imports then:

import { AuthMiddleware, LoggingMiddleware } from "middlewares";
// instead of
import { AuthMiddleware } from "../../../middlewares/auth-middleware";
import { LoggingMiddleware } from "../../../middlewares/logging-middleware";

Also you need to add and remove controllers each time (so you need to maintain index.ts file) which means extra boilerplate work.

As well as you need to manually remove the *.js file from build folder when you remove the TS file. Otherwise, the JS will stay and the deleted middleware will be still mounted in app. You need third party tools or script to clean the build folder on every recompilation as well as you can use ts-node or exe bundler like nexe when you rely on loading JS from directory.

I'm not against this feature if you both need it, but you heard my opinion on it.

We can use Object.values(middlewares), it's not a big deal but the PR would make this way of loading easy and official in docs. So, I would leave the support as you like it and use it in your apps but I would "fake" deprecate it to let people know that they use it on their own responsibility as it's not recommended and there are other ways to handle loading now. 😉

[pleerock]

As I understood your only arguments are:

• how to disable specific middleware
• how to handle order of middleware execution

Your first argument isn't an issue. You just need to comment @Middleware in your class, you don't need to comment entire class. And comment @Middleware is not a problem. Its easy and very logical and intuitive if you want to disable some middleware. And for sure its easier, more logical, more intuitive to do it this way, then go to specific index.ts find needed middleware and comment it.

Second argument... Yeah it will be much better (more logical and explicit) if we do middlewares this way: [A, B, C]. And people actually can do it this way if they prefer it. Or they can use "priorities" if they are satisfied by it. What you suggest does not give anything better. Having to specify [A, B, C] is still better then rely on order of imports in "index.ts". Having to rely on order of imports is also not intuitive. For most people import statements most of times just "to import something which do not have any side effects". Not intuitive and seamless with all its cons that this solution is better then priorities we have right now.

And your arguments are only about middlewares, because those arguments can't be applied to controllers (even first because in real life you don't need to disable specific controller because controllers aren't depend on each other).

[pleerock]

I always do index files as it's great in combination with tsconfig.json "Path mapping" feature and tsconfig-paths module - you can have nice readable imports then: import { AuthMiddleware, LoggingMiddleware } from "middlewares"; // instead of import { AuthMiddleware } from "../../../middlewares/auth-middleware"; import { LoggingMiddleware } from "../../../middlewares/logging-middleware";

yeah I understood that you use this technique so often, but I don't find it any useful. Even more - it makes things more complicated. I don't see any benifit in such approach, only cons I listed above

As well as you need to manually remove the *.js file from build folder when you remove the TS file. Otherwise, the JS will stay and the deleted middleware will be still mounted in app. You need third party tools or script to clean the build folder on every recompilation as well as you can use ts-node or exe bundler like nexe when you rely on loading JS from directory.

Good catch. Looks like thats the only pros. But this issue exist only on users who use outDir.

We can use Object.values(middlewares), it's not a big deal but the PR would make this way of loading easy and official in docs. So, I would leave the support as you like it and use it in your apps but I would "fake" deprecate it to let people know that they use it on their own responsibility as it's not recommended and there are other ways to handle loading now. 😉

Im not sure if I completely understood this comment, but if this feature will land it won't be "official way". It will be alternative way (for people like Michał Lytek who prefer this way over other not less worth ways 😉 )

[MichalLytek]

And for sure its easier, more logical, more intuitive to do it this way, then go to specific index.ts find needed middleware and comment it.

You can still go to the middleware source file and comment it, as you now do with dirs importing.

Second argument... Yeah it will be much better (more logical and explicit) if we do middlewares this way: [A, B, C]. And people actually can do it this way if they prefer it. Or they can use "priorities" if they are satisfied by it. What you suggest does not give anything better. Having to specify [A, B, C] is still better then rely on order of imports in "index.ts". Having to rely on order of imports is also not intuitive. For most people import statements most of times just "to import something which do not have any side effects". Not intuitive and seamless with all its cons that this solution is better then priorities we have right now.

I agree with that - index file isn't quite good for specifying order of middlewares. It works of course but the good way is using "array notation" as you said. However, the importing from index is really great for controllers and interceptors which don't have an order. For people who do index files for each category (services, entities, repositories, controllers, etc.) to have nice imports in apps and tests it's a no maintaining cost feature. I propose alternative loading, not the only one and remove other ways.

But this issue exist only on users who use outDir.

You complain about index.ts looks bad in folder structure but you don't use outDir and have , .js, .js.map and .d.ts files for each .ts source file in each folder? 😆

Im not sure if I completely understood this comment, but if this feature will land it won't be "official way". It will be alternative way (for people like Michał Lytek who prefer this way over other not less worth ways 😉 )

Official way = supported by framework (without dirty hacks), well described in docs, maintained by @NoNameProvided and @19majkel94 😉 And yes, an alternative way, not the only way, but also not hidden - user would know it exist by reading docs, not by digging in issues or source code.

BTW, this is really small changes, some kind of syntax sugar, but we discuss it so detailed and obstinately like we would debating about dropping of koa support...

[NoNameProvided]

Well lets go with both solution then.

• if there is a string, use it as a path to load from folder
• if object use our proposed change

@19majkel94 can you make a PR for this?

[pleerock]

You complain about index.ts looks bad in folder structure but you don't use outDir and have , .js, .js.map and .d.ts files for each .ts source file in each folder? 😆

I do use outDir. But if I don't then those files are collapsed in normal ide-s into single index.ts file 😉

[pleerock]

As I already told go ahead guys if you really need this way of importing 😄

[github-actions[bot]]

Stale issue message