[REQ] [typescript-inversify] Three small changes and one big?
Created by: bodograumann
We have adapted the typescript-inversify template for our own use. Some changes are definitely worth integrating into upstream. As per the contributing guidelines, let us discuss these changes. Pull request should follow.
I think for fixing these small issues, one pull request would be ok. I would also include a fix of #3618 (closed)
Falsy Required Parameters
When a required parameter is falsy, an error is thrown. But "" and false can be valid values. Cf. https://github.com/OpenAPITools/openapi-generator/blob/9cc7bd15f2884b12b373b172fa175063897724ff/modules/openapi-generator/src/main/resources/typescript-inversify/api.service.mustache#L63-L65
Multiple Media Types
The Accept header can only contain one media type in the current implementation. Cf. https://github.com/OpenAPITools/openapi-generator/blob/9cc7bd15f2884b12b373b172fa175063897724ff/modules/openapi-generator/src/main/resources/typescript-inversify/api.service.mustache#L138-L151
WithInterfaces
The template provides the withInterfaces option. When you enable it, apis.mustache still generates exports for the implementations, not the interfaces. Should this be changed? (Would be a breaking change I guess.)
Class Based Service Identificators
This is a major breaking change, so I'm unsure whether it should be integrated.
Currently the dependency injection provided by the template uses string based service identifiers. This means that type checking for the services has to be setup manually at the point of composition. Especially when interfaces are changing, this can lead to overlooked problems. See inversify issue 911 for a discussion.
The solution we came up with, is using abstract classes as service identifiers. This way the composition of the services changes from
container.bind<IHttpClient>("IApiHttpClient").to(HttpClient).inSingletonScope();
container.bind<IApiConfiguration>("IApiConfiguration").to(ApiConfiguration).inSingletonScope();to
container.bind(IHttpClient).to(HttpClient).inSingletonScope();
container.bind(IApiConfiguration).to(ApiConfiguration).inSingletonScope();and the types will be checked automatically.
After typescript compilation the abstract classes are still there, but because they don’t contain any implementation, they don’t use more resources than the strings.
I could create a separate pull-request for this change, but only if there is a genuine chance that it will be included.