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

feat: Add download file option #1699

Merged
merged 14 commits into from
May 10, 2022

Conversation

zachwhaley
Copy link
Contributor

What/Why/How?

Allows the user to set a download file name and format for the API spec.

For example, openapi.yaml

Reference

Fixes #672

Testing

New tests written

Screenshots (optional)

Check yourself

  • Code is linted
  • Tested
  • All new/updated code is covered with tests

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

Allows the user to set a download file name and format for the API spec.
@RomanHotsiy
Copy link
Member

RomanHotsiy commented Jul 30, 2021

How about just changing the default to openapi.yaml ?

@zachwhaley
Copy link
Contributor Author

@RomanHotsiy I believe there are still folks that would like the option to provide a JSON file as that is a valid format.

I did not change the default just to keep backwards compatibility.

@zcz3313
Copy link

zcz3313 commented Sep 9, 2021

@zachwhaley @RomanHotsiy can this PR be merged? our team needs this feature :)

Copy link
Member

@RomanHotsiy RomanHotsiy left a comment

Choose a reason for hiding this comment

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

I would implement downloadUrl instead (see my comments). Let us plz know if you would be open to do it 🙌

@@ -35,7 +41,13 @@ export class ApiInfoModel implements OpenAPIInfo {
}

if (IS_BROWSER && window.Blob && window.URL && window.URL.createObjectURL) {
const blob = new Blob([JSON.stringify(this.parser.spec, null, 2)], {
let specString: string;
if (path.extname(this.options.downloadFileName) === '.yaml') {
Copy link
Member

Choose a reason for hiding this comment

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

we ignore the yaml dumper as it adds a lot of code to the bundle: https://github.com/Redocly/redoc/blob/master/webpack.config.ts#L116

Can we instead implement a downloadUrl option so you can provide any URL and don't have Redoc to serialise the definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RomanHotsiy I'm not sure I understand what a downloadUrl option would entail. Are you suggesting users would provide their own URL for downloading a YAML API spec? If so, what if the URL they provide does not contain the API spec or is inaccessible?

I had originally hoped that the original API spec file used to create the webpage would be available to use for download, but that did not seem to be the case or I did not know where to look for it.

Copy link
Member

Choose a reason for hiding this comment

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

The original URL should work fine as long as you use URL and not use redoc-cli from a file on the filesystem (it should actually work like that but there may be a bug or something).

So I think it should work as below:

  • if URL was used in Redoc settings - the download button should just point to this URL
  • if a local file was used (e.g. redoc-cli) - it should use user-provided downloadUrl.
  • if a local file was used and no downloadUrl was provided, then we just serialize it to json and use openapi.json as filename (JSON is free in terms of bundle size vs yaml).

What do you think?

If so, what if the URL they provide does not contain the API spec or is inaccessible?

The purpose of this option is to provide the URL for download button. Why should anybody provide a bad URL there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I wasn't familiar with the multiple options for providing an API spec file; I believe our use case uses the redoc-cli.

So is there already an API spec URL that can be referenced in the code? Or are you suggesting this be added; if so, would this URL be used to generate the HTML or would it just be for downloading an API spec file?

Why should anybody provide a bad URL there?

I don't think it would be intentional, e.g. is it possible to use an internal or local company URL to generate the HTML pages? If so, that URL might not be accessible to the wider internet.

Co-authored-by: Roman Hotsiy <gotsijroman@gmail.com>
@adamdecaf
Copy link

Is there anything I can help with this to get it merged? Having a stable download URL would really help us auto-generate client code for multiple languages.

@RomanHotsiy RomanHotsiy added this to the v2.0 milestone Mar 22, 2022
@anastasiia-developer anastasiia-developer self-assigned this Apr 20, 2022
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ivana-isadora ivana-isadora left a comment

Choose a reason for hiding this comment

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

@anastasiia-developer Hi Anastasiia, here's my suggestion for the description, please take a look :)

@zachwhaley zachwhaley requested a review from a team as a code owner April 22, 2022 08:45
@zachwhaley zachwhaley requested review from zalesky and anastasiia-developer and removed request for a team April 22, 2022 08:45
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@anastasiia-developer
Copy link
Contributor

@zachwhaley Hi! Sorry for the huge delay. Thank you for your contribution to redoc.

Copy link
Collaborator

@AlexVarchuk AlexVarchuk left a comment

Choose a reason for hiding this comment

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

Looks nice. 🎉

@zachwhaley
Copy link
Contributor Author

Thanks! But removing the ability to download as yaml would mean #672 can’t fully be closed.

@RomanHotsiy
Copy link
Member

Thanks! But removing the ability to download as yaml would mean #672 can’t fully be closed.

We will have to close it as WON'T FIX. We don't want to add a heavy yaml dump dependency which will increase a bundle size for this use-case.

Maybe, we will consider something comparable: if the source was yaml we can save yaml too.

@anastasiia-developer anastasiia-developer merged commit b601c9a into Redocly:master May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

redoc-cli bundle downloaded spec filename and format
7 participants