Custom router wan't used #31
Conversation
src/server.ts
Outdated
| } | ||
| }); | ||
|
|
||
| this._app.use("/", this._router); |
There was a problem hiding this comment.
Probably hardcoding "/" here isn't the best part. Usually APIs have root namespace such as /api or smth. So probably additional configuration is needed. What do you guys think about that?
There was a problem hiding this comment.
I totally agree with that.
README.md
Outdated
| @@ -105,6 +105,16 @@ server.setErrorConfig((app) => { | |||
| }); | |||
| ``` | |||
|
|
|||
| ### `.setRoutingConfig(routingConfig)` | |||
There was a problem hiding this comment.
Can you please add a note about the default value { rootPath: "/" } being used here.
src/server.ts
Outdated
| @@ -25,6 +26,9 @@ export class InversifyExpressServer { | |||
| ) { | |||
| this._container = container; | |||
| this._router = customRouter || express.Router(); | |||
| this._routingConfig = { | |||
| rootPath: "/" | |||
There was a problem hiding this comment.
You probably need an extra additional optional argument for the default value.
There was a problem hiding this comment.
I'm not sure I've got your point here, do you mean provide default routing config or default root path via optional constructor argument?
There was a problem hiding this comment.
I think it is a good idea to add an optional constructor argument for routingConfig. I know they can use setRoutingConfig as well but I feel like is nicer to be able to configure everything via the constructor. What do you think? Thanks!
There was a problem hiding this comment.
Yeah, that sounds good to me. Besides there is no need for setRoutingConfig in this approach 👌
|
@maxmalov thanks for the PR 👍 I have added a couple of comments after doing the code review |
src/server.ts
Outdated
| @@ -96,9 +101,11 @@ export class InversifyExpressServer { | |||
| router[metadata.method](metadata.path, ...metadata.middleware, handler); | |||
| }); | |||
|
|
|||
| this._app.use(controllerMetadata.path, ...controllerMetadata.middleware, router); | |||
| this._router.use(controllerMetadata.path, ...controllerMetadata.middleware, router); | |||
There was a problem hiding this comment.
Oh, missed this newly created router. It should be replaced with custom router too.
|
@remojansen I've updated PR accroding to requested changes ☝️ |
|
@maxmalov thanks a lot for this PR 👍 |
Description
Related Issue
inversify/InversifyJS#486
Motivation and Context
inversify/InversifyJS#486
How Has This Been Tested?
Types of changes
Checklist: