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

chore(deps-dev): remove unused @testing-library/ deps #569

Merged

Conversation

agilgur5
Copy link

@agilgur5 agilgur5 commented Jul 20, 2024

Motivation

  • they're not used in the repo -- can search the repo for @testing-library and get no results other than the dep manifests
    • I guess I missed these in my previous removals (chore(deps): remove unused deps & resolutions #534) and assumed they were used or something
    • there aren't that many tests in this repo either
      • while testing-library is the preferred tool in the ecosystem these days, enzyme is still used in this repo
        • potentially could be replaced by testing-library (if no mounting etc is necessarily used), but not necessarily worthwhile given the deprecated state of this repo (argo-ui: future state of this repo #453)

Modifications

  • remove @testing-library/ deps from the package.json and yarn.lock via yarn remove

  • also automatically reorder the @types/uuid dep

    • happens automatically on a yarn add or yarn remove, I just left it out from previous removals as they were much larger and so it could be distracting

Verification

Build, lint, test, etc all still function fine as CI shows

@agilgur5 agilgur5 added type/dependencies Pull requests that update a dependency file javascript Pull requests that update Javascript code labels Jul 20, 2024
Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

I rebased locally and am getting errors from both yarn install and yarn start. Can you take a look?

- they're not used in the repo
  - I guess I missed these in my previous removals and assumed they were used or something
  - there aren't that many tests in this repo either
    - while `testing-library` is the preferred tool in the ecosystem these days, `enzyme` is still used in this repo
      - potentially could be replaced by `testing-library`, but not necessarily worthwhile given the deprecated state of this repo

- also automatically reorder the `@types/uuid` dep
  - happens automatically on a `yarn add` or `yarn remove`, I just left it out from previous removals as they were much larger and so it could be distracting

Signed-off-by: Anton Gilgur <[email protected]>
@agilgur5 agilgur5 force-pushed the chore-remove-unused-testing-library branch from fbdf536 to aa04e18 Compare August 22, 2024 22:10
@agilgur5
Copy link
Author

agilgur5 commented Aug 22, 2024

Rebased it myself here and it's working for me locally.
I'd guess your merge conflict resolution was not as precise as mine? I fixed the package.json and then git checkout --ours yarn.lock (to get the newest merged version) and then yarn to re-remove @testing-library/ and its deps

@agilgur5 agilgur5 requested a review from crenshaw-dev August 22, 2024 22:13
Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

Local tests look good!

@crenshaw-dev crenshaw-dev merged commit e76346f into argoproj:master Aug 23, 2024
5 checks passed
@agilgur5 agilgur5 deleted the chore-remove-unused-testing-library branch August 23, 2024 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code type/dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants