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

[typescript-fetch] Add initOverrides #10050

Merged

Conversation

badsyntax
Copy link
Contributor

@badsyntax badsyntax commented Jul 28, 2021

Fixes #4851

You can now pass in fetch initOverrides per request.

This change is mainly to add support for request cancellation (via AbortSignal), for example:

const controller = new AbortController();
const signal = controller.signal;

usersApi.getUser({ id: userId }, { signal });

About the target branch, it's not clear to me if i should be targeting master, but I guess this change can be released as a patch release as it's non-breaking.

@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny @topce @akehir @petejohansonxo @amakhrov

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master, 5.3.x, 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

You can now pass in fetch initOverrides per request.

This change is mainly to add support for request cancellation (via AbortSignal).
Copy link
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@macjohnny
Copy link
Member

@badsyntax thanks for your contribution!

@macjohnny macjohnny merged commit c4b9804 into OpenAPITools:master Aug 6, 2021
@macjohnny macjohnny added this to the 5.2.1 milestone Aug 6, 2021
@badsyntax
Copy link
Contributor Author

Thanks for the merge @macjohnny I have just noticed an issue though, see:

async deletePet(petId: number, , apiKey?: string, initOverrides?: RequestInit): Promise<void> {

I believe this should be a syntax error due to double commas (', ,')

I will follow up with another PR very shortly to fix this. Apologies for not spotting this.

@badsyntax
Copy link
Contributor Author

@macjohnny I've submitted a fix here: #10101

@mikkovor
Copy link

mikkovor commented Aug 26, 2021

In the 5.2.1 tag the runtime BaseAPI request looks like this:

    protected async request(context: RequestOpts, initOverrides?: RequestInit): Promise<Response> {
        const { url, init } = this.createFetchParams(context, initOverrides);
        const response = await this.fetchApi(url, init);
        if (response.status >= 200 && response.status < 300) {
            return response;
        }
        throw response;
    }

But when generated with this: npx @openapitools/openapi-generator-cli generate -i swagger.json -g typescript-fetch -o .\src\api

It is the one from previous versions:

 protected async request(context: RequestOpts): Promise<Response> {
    const { url, init } = this.createFetchParams(context);
    const response = await this.fetchApi(url, init);
    if (response.status >= 200 && response.status < 300) {
      return response;
    }
    throw response;
  }

Looks like the version should be correct:

Download 5.2.1 ...
Downloaded 5.2.1
Did set selected version to 5.2.1
[main] INFO  o.o.codegen.DefaultGenerator - Generating with dryRun=false
[main] INFO  o.o.codegen.DefaultGenerator - OpenAPI Generator: typescript-fetch (client)
[main] INFO  o.o.codegen.DefaultGenerator - Generator 'typescript-fetch' is considered stable.

This makes it so that it doesn't work since it generates the initOverrides but the request is the old one without the new added parameter.

Created the issue here

@badsyntax
Copy link
Contributor Author

@mikkovor Can you provide the swagger.json to allow me to replicate?

Can you also confirm you're definitely not using a custom template for runtime.mustache?

@badsyntax
Copy link
Contributor Author

I'm not able to replicate with:

npx @openapitools/openapi-generator-cli generate -i https://petstore.swagger.io/v2/swagger.json -g typescript-fetch -o ./src

@badsyntax
Copy link
Contributor Author

@mikkovor Can you also please delete the .\src\api directory and try re-generate? It looks like the generator won't overwrite existing files.

@mikkovor
Copy link

Yeah, looks like the script file had an innocent looking line at the git checkout src/api/runtime.ts which was the cause of this headache 🤦‍♂️ closing the issue

@simPod
Copy link
Contributor

simPod commented Dec 27, 2021

@badsyntax I'm at 5.3.1 and the runtime fetch generates still like protected async request(context: RequestOpts): Promise<Response> {

How do I make it protected async request(context: RequestOpts, initOverrides?: RequestInit): Promise<Response> { ?

@badsyntax
Copy link
Contributor Author

@simPod try delete the existing output dir first, as the generator won't overwrite existing files

@simPod
Copy link
Contributor

simPod commented Dec 27, 2021

@badsyntax I do it before each run of generate

@badsyntax
Copy link
Contributor Author

@simPod are you using a custom template perhaps?

@simPod
Copy link
Contributor

simPod commented Dec 27, 2021

Oh you're so right. Thanks!

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.

[REQ] Support AbortController in Typescript-Fetch
4 participants