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

[Feature] Add overloads in default templates for reportProgress/observe properties #114

Open
Lonli-Lokli opened this issue Jun 21, 2020 · 10 comments
Labels
enhancement New feature or request

Comments

@Lonli-Lokli
Copy link
Contributor

Example:


  public sendConfirmations(businessDate: string, confirmations?: Confirmation[], observe?: 'body', reportProgress?: boolean): Observable<SendConfirmationsResponse>;
  public sendConfirmations(businessDate: string, confirmations?: Confirmation[[], observe?: 'response', reportProgress?: boolean): Observable<HttpResponse<SendConfirmationsResponse>>;
  public sendConfirmations(businessDate: string, confirmations?: Confirmation[[], observe?: 'events', reportProgress?: boolean): Observable<HttpEvent<SendConfirmationsResponse>>;
  public sendConfirmations(businessDate: string, confirmations?: Confirmation[[], observe: any = 'body', reportProgress: boolean = false): Observable<any> {
@tajnymag
Copy link
Contributor

I don't think this should be handled by adding an "anonymous" boolean parameter for each request config option.

If so, how about having a single optional requestConfig with these as its attributes.

Just my 2 cents. I'm only a consumer here.

@luisfpg
Copy link
Contributor

luisfpg commented Jun 22, 2020

Really, reportProgress / observe are not currently handled by the generator.
We always generate a parameters object, so we could add a second argument with options.
The point is that this will be a significant amount of work, and I don't currently have time for it.
Will leave open.

@luisfpg luisfpg added the enhancement New feature or request label Jun 22, 2020
@Lonli-Lokli
Copy link
Contributor Author

@tajnymag with my proposal returned results will differ, so consumer will get correct result from calling code.
@luisfpg I can try to implement it with swagger 2.0 version as well if you want.

@tajnymag
Copy link
Contributor

@Lonli-Lokli I know. As long as RequestConfig interface differed, it would still be possible.

@luisfpg
Copy link
Contributor

luisfpg commented Jun 22, 2020

@Lonli-Lokli The swagger 2 sibling project (ng-swagger-gen) is no longer used or maintained by ourselves, only by community contribution, and it is getting harder and harder to maintain, as if I just plainly accept PR's it can break other things, and I really don't have a test project anymore for it.

For ng-openapi-gen, if you want to contribute a PR with passing tests, it would be more than welcome.

@Lonli-Lokli
Copy link
Contributor Author

@tajnymag can you please explain a little bit how different returned types can be done with single Params object?

@tajnymag
Copy link
Contributor

@Lonli-Lokli This works for me. resultFor(Body|Response|Events) gets their types correctly.

interface RequestConfig {
    observe?: 'body' | 'response' | 'events',
    reportProgress?: boolean
}
const RequestConfigDefault: RequestConfig = {
    observe: 'body',
    reportProgress: false
}

class ExampleService {
  public sendConfirmations(businessDate: string, confirmations?: Confirmation[], requestConfig?: RequestConfig & { observe?: 'body' }): Observable<SendConfirmationsResponse>;
  public sendConfirmations(businessDate: string, confirmations?: Confirmation[], requestConfig?: RequestConfig & { observe?: 'response' }): Observable<HttpResponse<SendConfirmationsResponse>>;
  public sendConfirmations(businessDate: string, confirmations?: Confirmation[], requestConfig?: RequestConfig & { observe?: 'events' }): Observable<HttpEvent<SendConfirmationsResponse>>;
  public sendConfirmations(businessDate: string, confirmations?: Confirmation[], requestConfig = RequestConfigDefault): Observable<any> {
    if (requestConfig.observe === 'response') {
        return new Observable<HttpResponse<SendConfirmationsResponse>>();
    }

    if (requestConfig.observe === 'events') {
        return new Observable<HttpEvent<SendConfirmationsResponse>>();
    }

    return new Observable<SendConfirmationsResponse>();
  }
}

const example = new ExampleService();
const resultForBody = example.sendConfirmations('2020-22-06', [], { observe: 'body' });
const resultForResponse = example.sendConfirmations('2020-22-06', [], { observe: 'response' });
const resultForEvents = example.sendConfirmations('2020-22-06', [], { observe: 'events' });

@Lonli-Lokli
Copy link
Contributor Author

@tajnymag got it, thanks!

@Lonli-Lokli
Copy link
Contributor Author

Seems like its more difficult than I thought as current solution is based on pure HttpRequest usage :(
It requires deep rewrite based on implementation of HttpClient and not sure I will have enough time.

@Lonli-Lokli
Copy link
Contributor Author

Lonli-Lokli commented Jan 9, 2021

@luisfpg going back, I was sticker with RequwstBuilder as it does not give full control on props and returned types.
Also with new approach it will be required to allow 4 different returned types which will be obviously breaking change. Not sure if we will need second method, $Response which currently supposed to be used to get access to returned headers.
Can you share your ideas behind this?

That's how it's done in competitor https://github.com/OpenAPITools/openapi-generator/blob/9bd2a45e72bd4e65c22ae3f3ce4a1ff8bd8db8b9/modules/openapi-generator/src/main/resources/typescript-angular/api.service.mustache

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants