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

API client, use object not arg variables #589

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

bakesteve
Copy link
Contributor

Not sure if there is a neater way to do this, and may want to make this optional, but this opens up the capabilities of spread operators and the like to any complex api client calls.
so if I start with an api that can take a number of options on a GET call, I would currently get each parameter as an argument variable.
This moves it to always generate with a single argument object
So we move from get(id: int, from: Date, to: Date)
to get(q: {id: int, from: Date, to: Date})
which opens up all the goodness of object destructuring, spread operators, and other es6 features

one alternate approach would be to only do this based on the original API signature in the web api controller.
so:

[HttpGet]
[Route("{rateId:int}")]
[ResponseType(typeof(Shared.DTOs.Thing))]
public IHttpActionResult GetById(long id) {

}

[HttpGet]
[Route("Get")]
[ResponseType(typeof(Shared.DTOs.Thing))]
public IHttpActionResult Get([FromUri]SingleQueryModel lookupObj) {
}        

would generate

getById(id: number): Promise<Thing>;
get(lookupObj: SingleQueryModel): Promise<Thing>;
// rather than the current
get(id: number, otherId: number, workDate: moment.Moment, dimensionIds: string, ...etc): Promise<Thing>;

which I think may be a better approach, but coudlnt immediately see how to make that happen?

Steve Baker added 3 commits February 12, 2017 17:33
So we move from get(id: int, from: Date, to: Date)
to get(q: {id: int, from: Date, to: Date})
which opens up all the goodness of object destructuring, spread operators, and other es6 features
@bakesteve
Copy link
Contributor Author

this actually only needs aff7013
so if you want me to rebase I can

@RicoSuter
Copy link
Owner

This is a major breaking change in the generated code...

@bakesteve
Copy link
Contributor Author

@RSuter sure - probably best off as an issue?
would this be something we could add as a option (false by default) to avoid the breaking nature?
or is there anyway you can think to allow us to inject something (ideally as part of an msbuild step)? Given the issues with t4 and command line, and the fact that we'd rather not fork the rest of the template, not sure what the best approach is
but totally get the reasons not to do it as is and not behind some config

@RicoSuter
Copy link
Owner

Yes, please create an issue and reference this pr. We will discuss this there...

@grovesNL
Copy link
Contributor

@bakesteve: Great idea, I thought about doing this a while back but didn't get around to implementing it. However as you said above, I think this needs to be an opt-in feature to avoid the breaking change.

You can also avoid forking the templates too much by destructuring inside the parameter list of the function declaration, i.e. {a, b} below:

function get({ a, b }) { console.log(a, b); }

so get({a: 1, b: 2}) outputs 1 2

This would remove the need to write q. before each variable name so the template changes are smaller.

@oguzhankahyaoglu
Copy link

Hello there,
Are the any progress on this issue? I needed this functionality today :)

@JonasMee
Copy link

@RicoSuter Any news on this?

@ghost
Copy link

ghost commented Aug 17, 2020

@RicoSuter any news on this? :-)

@eulergkb
Copy link

eulergkb commented Nov 3, 2023

Found this package to be useful for generating typescript client: https://www.npmjs.com/package/swagger-typescript-api - Please avoid using NSwag ts client generation as it makes your codebase harder to maintain

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

Successfully merging this pull request may close these issues.

6 participants