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

docs: update SwaggerUI version #12518

Merged
merged 2 commits into from
Feb 9, 2024
Merged

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Jan 14, 2024

Follow-up to #10923, where the version of Swagger UI added there was already a year+ old at the time of the PR

Motivation

Modifications

  • update the SwaggerUI dep loaded in a script tag on the docs site's Swagger page

  • significantly simplify the HTML code block as it renders inside of existing MD/HTML

    • a second DOCTYPE, html, head, body, etc tags are actually invalid HTML, but either mkdocs ignored some of those or browsers just ignore some of those
      • fixing invalid HTML makes this doc page more robust
    • also fix the indentation in the code block too, which also did not meet style guidelines
  • add a markdownlint-disable comment to allow the inline HTML

    • add a comment describing why it's necessary
      • similarly add a comment to the PR template explaining why that one is necessary too
    • remove the Makefile's ignore on swagger.md for docs linting et al

Verification

  1. make docs still passes
    • including docs-lint, docs-spellcheck, etc
  2. open site/swagger/index.html renders fine

- use latest [5.11.0](https://github.com/swagger-api/swagger-ui/releases/tag/v5.11.0) instead of the 2 years outdated [4.5.0](https://github.com/swagger-api/swagger-ui/releases/tag/v4.5.0)
  - it seems like the version was taken directly from [the SwaggerUI installation docs](https://github.com/swagger-api/swagger-ui/blob/v5.11.0/docs/usage/installation.md#unpkg), which still has an old version listed
    - in fact, the entire code block was taken from the doc
      - significantly simplify the code block as it renders inside of existing MD/HTML
        - a second `DOCTYPE`, `html`, `head`, `body`, etc tags are actually invalid HTML, but either `mkdocs` ignored some of those or browsers just ignore some of those
          - fixing invalid HTML makes this doc page more robust
      - also fix the indentation in the code block too, which also did not meet style guidelines

- this seems to make it slightly more performant, which is our main issue, but also has other benefits of using a current version of a dep

- add a `markdownlint-disable` comment to allow the inline HTML
  - add a comment describing why it's necessary
    - similarly add a comment to the PR template explaining why that one is necessary too
  - remove the `Makefile`'s ignore on `swagger.md` for docs linting et al

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 added type/dependencies PRs and issues specific to updating dependencies area/docs Incorrect, missing, or mistakes in docs javascript Pull requests that update Javascript dependencies labels Jan 14, 2024
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

this seems to make it slightly more performant, which is our main issue

What performance issue?

@agilgur5
Copy link
Contributor Author

agilgur5 commented Jan 14, 2024

What performance issue?

Try opening any API method in the Swagger UI and watch it lag, freeze, or straight up crash the browser window.

It's a very prominent issue and was even mentioned in #10923, the original PR that added this (as linked at the top of the description). JM briefly mentioned it on Slack too and I agreed there that it is very hard to use in its current state due to the lag etc.
There's also some historical issues on the performance of the Swagger UI within the Server's UI as well: #6645 (this PR is for the docs, but same issues as they both have the same giant Swagger file and use Swagger UI)

As I wrote, this seems to make it better, but does not fix it. Anecdotally, it didn't crash on me once I made the update and it froze the whole page less often - i.e. it mainly lagged, which is the least bad UX effect of the 3 performance issues seen.
It likely needs more performance related fixes upstream, but those will only come in v5+ regardless

Copy link
Member

@isubasinghe isubasinghe left a comment

Choose a reason for hiding this comment

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

Nice work, this actually makes the UI a lot more usable.

Signed-off-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
@agilgur5
Copy link
Contributor Author

agilgur5 commented Feb 9, 2024

Fixed merge conflict

@agilgur5
Copy link
Contributor Author

agilgur5 commented Feb 9, 2024

Gonna merge this in as it has already been approved and is a nice perf boost for the docs. I also already upgraded the UI to v5 in #12540, which required a more careful process.

@agilgur5 agilgur5 enabled auto-merge (squash) February 9, 2024 22:05
@agilgur5 agilgur5 merged commit 2e9c1ca into argoproj:main Feb 9, 2024
27 checks passed
@agilgur5 agilgur5 deleted the docs-update-swagger-ui branch February 9, 2024 22:25
agilgur5 added a commit that referenced this pull request Oct 27, 2024
- fixed one merge conflict with the `pull_request_template.md` where the Sustainability Effort mention was not backported to release-3.5

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
(cherry picked from commit 2e9c1ca)
@agilgur5
Copy link
Contributor Author

Backported this update into release-3.5 in f4470ed. Will probably do the same for release-3.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Incorrect, missing, or mistakes in docs javascript Pull requests that update Javascript dependencies type/dependencies PRs and issues specific to updating dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants