-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
Ft/3832 add switches for disabling rendering of large payloads #9625
base: master
Are you sure you want to change the base?
Conversation
f64f09e
to
f088944
Compare
Thanks! |
EDIT: I found one of my old comments in the topic: #3832 (comment)
if github has a max so should swagger-ui maybe this is a good start: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I considered setting some default value, but decided against it because I thought it would (technically) count as a breaking change. Should we consider it a bug fix/feature, even though someone might be used to getting pretty print after a long wait time? |
After the long wait time the pretty print is not usable in most browsers, often preventing proper page display, so I'd say it would count as a bugfix. Not displaying the document at all may be a breaking change though. |
Yes I think so, this can be considered a bug fix, most reported cases the browser stops responding |
Seems no one from the team is looking at these PR to give feedback... |
@char0n can we get a review on this PR? |
Hi @heldersepu, Yes, @glowcloud will take over this one. I have a small issue with naming:
|
Hello @char0n ... @kristiansamuelsson this is your PR you are almost there ... If I don't hear from you I will try make the changes this weekend |
@heldersepu |
a875658
to
43a3665
Compare
I have updated the config name based on the suggestion from @char0n |
I can confirm that changing that parameter in settings is fine but it doesn't seem to work in the URL. Indeed, it looks like a new issue. Created a separate issue for it: #9674 |
I would say yes, but for that we can create another issue which will be an enhancement to this one. The primary goal here not the crash the UI
Depends - does it crash the UI in the same way as rendering the example? |
I love the lively discussion!
|
@kristiansamuelsson this PR shows: |
43a3665
to
72e7032
Compare
Hi @kristiansamuelsson, we'll use parts of this PR and we want to go with general direction as described in my comment and implemented in your PR. The final implementation will be a bit different, utilizing SwaggerUI plugin system and |
Description
Adds two configuration options to control if/how payloads gets rendered, depending on the size of the payload.
renderSizeThreshold
)syntaxHighlight.sizeThreshold
)Motivation and Context
If a response is too large, the UI hangs for a very long time - sometimes crashes. This is discussed in both #3832 and #4018.
I am not entirely sure if these options counts as solving those issues, but at least it provides a way of letting users download large responses without waiting for the UI to render, and avoids crashes of even larger responses.Edit (see comments below):
Fixes #3832
Fixes #4018
How Has This Been Tested?
Added unit tests to cover the new functionality.
Also tested on a simple service running locally, which allowed me to control the size of the response. I configured
renderSizeThreshold
to6000000
(~6MB) andsyntaxHighlight.sizeThreshold
to 1000000 (~1MB). I could see that responses with size just under 6MB were very slow, and responses just over 6MB were very quick (but response is not displayed). Responses just under 1MB were very slow, and responses just over 1MB significantly quicker.Screenshots (if appropriate):
Checklist
My PR contains...
src/
is unmodified: changes to documentation, CI, metadata, etc.)package.json
)My changes...
Documentation
Automated tests