You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
Falsy Required Parameters
When a required parameter is falsy, an error is thrown. But "" and false can be valid values. Cf.
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
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.
The text was updated successfully, but these errors were encountered:
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
Falsy Required Parameters
When a required parameter is falsy, an error is thrown. But
""
andfalse
can be valid values. Cf.openapi-generator/modules/openapi-generator/src/main/resources/typescript-inversify/api.service.mustache
Lines 63 to 65 in 9cc7bd1
Multiple Media Types
The Accept header can only contain one media type in the current implementation. Cf.
openapi-generator/modules/openapi-generator/src/main/resources/typescript-inversify/api.service.mustache
Lines 138 to 151 in 9cc7bd1
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
to
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.
The text was updated successfully, but these errors were encountered: