typescript-fetch: Fix compile error #6538
Conversation
Fixes an annoying compile error that happens when a .NET Core 3.1 Swashbuckle generated `swagger.json` has a Controller with a request class.
> Type 'ApproveBrandRequest[]' is not assignable to type 'string | number | boolean | HTTPQuery | (string | number | boolean)[]'.
Type 'ApproveBrandRequest[]' is not assignable to type '(string | number | boolean)[]'.
Type 'ApproveBrandRequest' is not assignable to type 'string | number | boolean'.
Type 'ApproveBrandRequest' is not assignable to type 'true'.
|
The build failure seems to be transient from the description, and should be able to be ignored. |
|
cc @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02) |
|
@ffMathy please merge the latest master to address some of the CI errors. |
|
Please also run |
|
I re-merged |
|
You will need to install Git BASH (https://gitforwindows.org/) in order to run the script on Windows. |
|
the typing should be ok, maybe it is an object that doesn't fit that type that gets assigned? @ffMathy could you please provide the generated code of the service and an example of the the input parameter? |
|
@macjohnny I have shown the bad code below. The generated export interface ApproveBrandRequest {
/**
*
* @type {string}
* @memberof ApproveBrandRequest
*/
brand?: string | null;
/**
*
* @type {string}
* @memberof ApproveBrandRequest
*/
category?: string | null;
/**
*
* @type {string}
* @memberof ApproveBrandRequest
*/
country?: string | null;
}
export interface BrandsApprovePostRequest {
requests?: Array<ApproveBrandRequest> | null;
}The C# class looks like this: public class ApproveBrandRequest
{
public string Brand { get; set; } = null!;
public string Category { get; set; } = null!;
public string Country { get; set; } = null!;
}The controller method looks like this: [Route("brands/approve")]
[HttpPost]
public async Task<IActionResult> ApproveBrand(ApproveBrandRequest[] requests)
{
//...
return Ok();
}The violating TypeScript code that was generated (and fails compilation) is this: async brandsApprovePostRaw(requestParameters: BrandsApprovePostRequest): Promise<runtime.ApiResponse<void>> {
const queryParameters: runtime.HTTPQuery = {};
if (requestParameters.requests) {
queryParameters['requests'] = requestParameters.requests; // <-- this line gives the error.
}
const headerParameters: runtime.HTTPHeaders = {};
const response = await this.request({
path: `<relative URL removed for obscurity>/brands/approve`,
method: 'POST',
headers: headerParameters,
query: queryParameters,
});
return new runtime.VoidApiResponse(response);
} |
|
@wing328 thanks, but my problem is that the given file is not present. |
|
But @macjohnny why insist on having typing within a method where that is not used in the first place? The typings inside that file are not necessary 🙂 We're basically adding unnecessary strictness that (at best) can't do anything positive, and (at worst) can only make things worse. Typings are present to help the developer catching type errors - right? But since we're using Mustache, there's absolutely no reason for it IMO. I get that the typings are present towards the consumer of the method. But internal variables inside the method should jus the |
|
@ffMathy the typings in the .mustache file help to identify problems in the generated code, so they can be considered useful. please merge the current master into your branch, so you can run |
|
@macjohnny at what point do they identify the issues though? Is it because As you can see in the GitHub history, I already merged master into my branch, and the file is still not there. I'll try doing it again though, and report back. |
merge master
|
Alright, that worked - the file is there now. I must have missed something or pulled too early. Now I can't build the project though. I'm running |
|
try running |
|
Thank you so much. That helped. It has been updated now! 🙂 |
| docs\AdditionalPropertiesBoolean.md | ||
| docs\AdditionalPropertiesClass.md | ||
| docs\AdditionalPropertiesInteger.md | ||
| docs\AdditionalPropertiesNumber.md |
There was a problem hiding this comment.
the file path separator seems to differ between windows and linux, please revert this
cc @wing328
There was a problem hiding this comment.
Yeah I'm gonna make a whole different PR to fix this because it has left such a big mess 🙁
Some hard honest feedback: I must say, the experience of contributing to this PR has been a nightmare so far. I think more people would contribute if it was easier. This bug has existed for 5+ months, and I am sure many people have it - yet no one has contributed a fix yet. I suspect that's because it's very difficult.
The project is great though! 🙂
But which parts do I remove, and which parts do I keep? I have no idea.
There was a problem hiding this comment.
@ffMathy sorry for hear that. We're in transition to a new process to update the samples.
Please PM me via https://join.slack.com/t/openapi-generator/shared_invite/enQtNzAyNDMyOTU0OTE1LTY5ZDBiNDI5NzI5ZjQ1Y2E5OWVjMjZkYzY1ZGM2MWQ4YWFjMzcyNDY5MGI4NjQxNDBiMTlmZTc5NjY2ZTQ5MGM tomorrow (Thur) when you've time.
There was a problem hiding this comment.
@ffMathy I agree it is not straightforward to implement changes in this project. re-generating the samples was re-factored recently, so apparently there are still some issues with windows support.
please list the issues you have faced when contributing, possibly with a suggestion how to improve them
There was a problem hiding this comment.
Sorry, I can't contribute much to anything else than solving this issue, since I am a consultant and I am being paid to work for a company. They allowed me to fix the bug that causes them to be stuck, but not more than that.
I hope you understand. Can you let me know when the Windows build script has been fixed, so I can regenerate using that?
There was a problem hiding this comment.
I just updated the samples via 5a754e3. Let's see how it goes.
|
@ffMathy I just noticed that this should fix your issue: change to could you please test that and change your PR accordingly? |
I think it might need a generic type, something along the lines of: // runtime.mustache#L187
export type HTTPQuery<T = void> = { [key: string]: string | number | null | boolean | T | Array<string | number | null | boolean | T> | HTTPQuery};
// apis.mustache#L101
const queryParameters: HTTPQuery<typeof requestParameters.['{{paramName}}']> = {};That should hopefully result in the following ts code: export type HTTPQuery<T = void> = { [key: string]: string | number | null | boolean | T | Array<string | number | null | boolean | T> | HTTPQuery};
export interface ApproveBrandRequest {
/**
*
* @type {string}
* @memberof ApproveBrandRequest
*/
brand?: string | null;
/**
*
* @type {string}
* @memberof ApproveBrandRequest
*/
category?: string | null;
/**
*
* @type {string}
* @memberof ApproveBrandRequest
*/
country?: string | null;
}
export interface BrandsApprovePostRequest {
requests?: Array<ApproveBrandRequest> | null;
}
export class ApproveBrandRequest {
async brandsApprovePostRaw(requestParameters: BrandsApprovePostRequest): Promise<void> {
const queryParameters: HTTPQuery<typeof requestParameters['requests']> = {};
if (requestParameters.requests) {
queryParameters['requests'] = requestParameters.requests; // <-- this line gives the error.
}
}
} |
|
@macjohnny I left a comment to your review question. |
|
I merge this now to have a working fix for the issue, but the typing should be fixed properly in a separate PR. The solution proposed by @akehir sounds good. |
Fixes an compile error that happens when a .NET Core 3.1 Swashbuckle generated
swagger.jsonhas a Controller with a request class.PR checklist
./bin/(or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run./bin/{LANG}-petstore.sh,./bin/openapi3/{LANG}-petstore.shif updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).master,4.3.x,5.0.x. Default:master.