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

refactor(deps)!: remove swagger-ui-react #13818

Merged
merged 8 commits into from
Oct 30, 2024

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Oct 25, 2024

Partial fix for #13410 -- see #13821 for the other half to downgrade react that is a pure fix (vs this is a breaking change that is sizeable in its own right)
Replaces #13069 to resolve CVEs by just removing swagger-ui-react entirely

Motivation

Removes the Swagger UI entirely from the UI as it is a gigantic dep (2nd largest only behind Monaco Editor) and often has vulnerable dependencies that are difficult to resolve without affecting other deps. According to #12061, this would shave off ~2MB / 826KB minified / 250KB gzipped from the UI bundle and will also speed up builds and installs as such.

It also no longer necessary to have in the UI as there is a Swagger UI in the docs after #10923 and the docs are versioned after #11390

Modifications

  • remove swagger-ui-react dep
    • and @types/swagger-ui-react devDep
  • replace /apidocs UI page with a few links: download the OpenAPI / Swagger spec, download the JSON schema, and go to the Swagger UI in the versioned documentation
  • replace other UI links to the API Docs with versioned docs links as well
    • could leave them as-is, but they seem to be intended for use with the Swagger UI
    • ensure target='blank' and rel='noreferrer' consistent with existing links in the UI

Docs update

update and simplify REST API page

  • client auth and access tokens are already described in other pages, can remove these and just link
  • remove bullet about interactive usage in the UI
  • update bullet about the swagger reference as it is always up-to-date (it pulls from main) and can be versioned now as well

Verification

Manually tested and confirmed the page works as intended. Screenshot of new API docs page:
Screenshot 2024-10-25 at 4 03 36 PM

Future Work

  1. Downgrade React to fix a bunch of compat warnings (and unexpected errors like fix(ui): handle React 18 batching for "Submit" button on details page. Fixes #13453 #13593) etc per above linked javascript dependency warnings -- mostly due to incompatible React  #13410, fix(deps): upgrade swagger-ui-react v5.17.3, react-dom v18.3.1, and react v18.3.1. Fixes CVEs #13069 (comment), etc
  2. Have the versioned API reference page pull its reference from the same version GH, instead of main

@agilgur5 agilgur5 added type/security Security related type/dependencies PRs and issues specific to updating dependencies area/ui javascript Pull requests that update Javascript dependencies labels Oct 25, 2024
@agilgur5 agilgur5 added this to the v3.6.0 milestone Oct 25, 2024
@agilgur5 agilgur5 changed the title refactor(deps)!: remove swagger UI refactor(deps)!: remove swagger-ui-react Oct 25, 2024
@agilgur5 agilgur5 added the area/docs Incorrect, missing, or mistakes in docs label Oct 25, 2024
@agilgur5 agilgur5 marked this pull request as ready for review October 25, 2024 20:43
@tooptoop4
Copy link
Contributor

if swagger ui is removed from each operator's server should there be an entry in breaking changes/upgrading guide?

@agilgur5
Copy link
Contributor Author

It is breaking, so we could, although the UI itself tells you what to do and there is no upgrade instruction otherwise. The impacted UI page having a message for a deprecation window for the Archived List UI could have made sense too, i.e. a better option than #13371

I'm inclined to agree and note it in this case, although we do want to be careful about making the UI a "breakable" surface area and where to draw boundaries as it's not quite the same as the schema / API / etc "Public API" (also I noticed metrics are missing there) but rather a consumer of those.

@Joibel
Copy link
Member

Joibel commented Oct 28, 2024

Could this be added to docs/upgrading.md to note the removal of the feature?

- replace with a versioned link to the Swagger UI in the Docs
  - and downloads of the current API spec and JSON schema to use your exact version locally

- Note: the `download` doesn't seem to be working in my browser for some reason...

Signed-off-by: Anton Gilgur <[email protected]>
- could leave them as-is, but they seem to be intended for use with the Swagger UI

Signed-off-by: Anton Gilgur <[email protected]>
- client auth and access tokens are already described in other pages, can remove these and just link
- remove bullet about interactive usage in the UI
- update bullet about the swagger reference as it is always up-to-date (it pulls from `main`) and can be versioned now as well

Signed-off-by: Anton Gilgur <[email protected]>
- shaves off a ton from the bundle (2nd biggest dep, only after Monaco Editor)
- resolves CVEs that were specific to Swagger UI's deps by just removing them entirely

Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
@agilgur5
Copy link
Contributor Author

Added an upgrading note in d706239

Copy link
Member

@Joibel Joibel left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for sorting this one

@Joibel
Copy link
Member

Joibel commented Oct 29, 2024

@agilgur5, could you resolve the conflicts and then we can merge and cut a 3.6.0-rc4 with this in?

@MasonM
Copy link
Contributor

MasonM commented Oct 30, 2024

@Joibel I can take this over if you'd like, though I don't have access to push to https://github.com/agilgur5/argo-workflows/tree/refactor-remove-swagger-ui (which I'm assuming I'll get when my membership request goes through).

I'm assuming the best way forward is to create a new PR with a branch forked from https://github.com/agilgur5/argo-workflows/tree/refactor-remove-swagger-ui. Does that sound reasonable?

@agilgur5
Copy link
Contributor Author

agilgur5 commented Oct 30, 2024

It requires write access on GH to push to PR, or Approver in Argoproj terms.

I just added you as a collaborator to my fork for simplicity

I'm assuming the best way forward is to create a new PR with a branch forked from

Otherwise that would be the general process that has been done in the past and tends to be an easy way to give proper attribution

@MasonM
Copy link
Contributor

MasonM commented Oct 30, 2024

@agilgur5 Thanks! It works, and I resolved the conflicts

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.

Thanks!

@terrytangyuan terrytangyuan merged commit 3bbec4e into argoproj:main Oct 30, 2024
16 checks passed
@agilgur5 agilgur5 deleted the refactor-remove-swagger-ui branch October 30, 2024 04:42
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 area/ui javascript Pull requests that update Javascript dependencies type/dependencies PRs and issues specific to updating dependencies type/security Security related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants