Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REQ] [typescript-inversify] Three small changes and one big? #3838

Closed
bodograumann opened this issue Sep 5, 2019 · 3 comments · Fixed by #3849
Closed

[REQ] [typescript-inversify] Three small changes and one big? #3838

bodograumann opened this issue Sep 5, 2019 · 3 comments · Fixed by #3849

Comments

@bodograumann
Copy link
Contributor

bodograumann commented Sep 5, 2019

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.

if (!{{paramName}}){
throw new Error('Required parameter {{paramName}} was null or undefined when calling {{nickname}}.');
}

Multiple Media Types

The Accept header can only contain one media type in the current implementation. Cf.

{{^produces}}
headers['Accept'] = 'application/json';
{{/produces}}
{{#produces.0}}
headers['Accept'] = '{{{mediaType}}}';
{{/produces.0}}
{{#bodyParam}}
{{^consumes}}
headers['Content-Type'] = 'application/json';
{{/consumes}}
{{#consumes.0}}
headers['Content-Type'] = '{{{mediaType}}}';
{{/consumes.0}}
{{/bodyParam}}

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.

@auto-labeler
Copy link

auto-labeler bot commented Sep 5, 2019

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@macjohnny
Copy link
Member

cc @gualtierim

@gualtierim
Copy link
Contributor

I prefer an unique request that summarise all. For the first request, @macjohnny in the other javascript based languages only undefined is checked?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants