Skip to content

[typescript-fetch] Add support for File (application/octet-stream) response types #1367

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

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

Conversation

spallister
Copy link

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.
    @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @nicokoenig (2018/09) @topce (2018/10)

Description of the PR

This is a copy of a similar PR I submitted to the swagger-codgen repo here. A while back, @wing328 reached out to let me know the majority of the community moved over here, where I might want to consider re-submitting my PR.

The purpose of this PR is to return application/octet-stream response types as Files, instead of processing them as JSON. In the PR I had submitted to the swagger-codegen repo, I was returning such response types as Blobs, but soon after had a need to obtain the file's name (i.e. which File supports, but Blob does not).

Note that this PR includes a change for DefaultGenerator.java, as I needed to add a property to the mustache template model to determine whether the API contained at least one 'File Response'-type Operation. In particular, this was needed so that I could conditionally include a responseToFile helper method within api.mustache as necessary (i.e. our TS linter would otherwise throw an unused method warning for applications where no 'File Response'-type Operations were in use).

It looks like there is a TypeScript-fetch rewrite taking place over at #569, but figured I'd submit this in case it can be merged in the interim, or incorporated into that rewrite.

Fixes #531

@wing328
Copy link
Member

wing328 commented Dec 7, 2018

@spallister I'll review this change over the weekend and let you know if I've any question. I may need your help to resolve the merge conflicts.

@macjohnny
Copy link
Member

reminder: conflicts should be resolved

@macjohnny
Copy link
Member

@spallister @wing328 ping

@wing328
Copy link
Member

wing328 commented Mar 25, 2019

@spallister can you please resolve the merge conflicts when you've time? then we'll merge it into master if no one has further feedback on this PR.

@wing328 wing328 modified the milestones: 4.0.0, 4.0.1 May 13, 2019
@wing328 wing328 modified the milestones: 4.0.1, 4.0.2 May 31, 2019
@wing328 wing328 modified the milestones: 4.0.2, 4.0.3 Jun 20, 2019
@wing328 wing328 modified the milestones: 4.0.3, 4.1.0 Jul 9, 2019
@wing328 wing328 modified the milestones: 4.1.0, 4.1.1 Aug 9, 2019
@wing328 wing328 modified the milestones: 4.1.1, 4.1.2 Aug 26, 2019
@wing328 wing328 modified the milestones: 4.1.2, 4.1.3 Sep 11, 2019
@wing328 wing328 modified the milestones: 4.1.3, 4.2.0 Oct 4, 2019
@wing328 wing328 removed this from the 4.2.0 milestone Oct 30, 2019
@deefdragon
Copy link

Is this still viable in 5.x/6.0 or have too many changes been made since this was introduced to merge it in?

@wlievens
Copy link

wlievens commented Jun 9, 2023

Is there any news for this feature? Being able to use the generated API for downloading blobs seems essential.

@TiFu
Copy link
Contributor

TiFu commented Jun 14, 2023

@wlievens: This PR would unfortunately need to be updated before it can be accepted given the merge conflicts and submission ~5 years ago (not sure if anything substantially changed that would require changes to this PR's implementation).

If you have some time on your hands, we would of course welcome a PR resolving this :)

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.

[typescript-fetch] Add support for file (application/octet-stream) response types
6 participants