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(ui): migrate UserInfo to functional component #11793

Merged

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Sep 10, 2023

Partial fix for #9810
Partial fix for #11740

Motivation

  • Use newer, modern functional components compatible with hooks instead of legacy class-based components

  • Use newer, modern async/await syntax instead of Promise chains

  • also remove a deprecated usage of BasePage

    • BasePage is a deprecated usage of react-router's private context before React made context an official API (it's quite old)
      • removing this is necessary to upgrade react-router-dom as the old, private context no longer exists in new versions
    • I'm also in process of migrating its remaining usages to functional components, this is one of a few PRs to do so (the others are significantly more complex, so I have split them out. See refactor(ui): migrate Reports to functional component and split files #11794)

Modifications

  • mostly straightforward state refactor from this.state to useState

    • plus refactor componentDidMount to useEffect
  • remove deprecated BasePage extends and typings

    • it wasn't even used in UserInfo (or at least not currently), so just removed references
      • no routing primitives are used in UserInfo
  • plus remove unused BasePage methods

    • these are not used by anything at this time
      • searched the codebase and found 0 references

Verification

  • Pure refactor, no real semantic changes
  • Manually tested locally that the UserInfo page works the same as it did before

Notes for Reviewers

Turn on "ignore whitespace" to make the diff much simpler to read

- mostly straightforward state refactor
- plus refactor `componentDidMount` to `useEffect`

- also remove the deprecated usage of BasePage
  - BasePage wasn't even used in the component (or at least not currently), so just the `extends` and typings were removed, effectively
  - BasePage is a deprecated usage of `react-router`'s private context before React made context an official API (it's quite old)
    - UserInfo did not use any routing primitives

- manually tested and works the same as it did before

Signed-off-by: Anton Gilgur <[email protected]>
- these are not used by anything at this time
  - searched the codebase and found 0 references

Signed-off-by: Anton Gilgur <[email protected]>
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.

Can we merge similar PRs?

@agilgur5
Copy link
Contributor Author

agilgur5 commented Sep 11, 2023

Can we merge similar PRs?

At your previous request, I did. You picked the odd PR out.

#11791 has 12 components and 27 files changed. They are all pure functional components or have very minimal state.
I mentioned there in the opening comment your previous request as well -- I merged them specifically at your request.

This PR has an effect, unlike all the rest in #11791. (I also wrote it today whereas #11791 is from yesterday)

Per my opening comment, I split out #11794 from this PR as #11794 is substantially more complex than any of the rest (and also took me by far the longest to rewrite). It also follows the structure of an older PR closely for ease of review.

In any case, all 3 pass CI as there are significantly less flakes after #11753. They all merge cleanly as well. So at this point, it would also be unnecessary extra work to merge them together, especially when that makes the diff much worse and harder to review as well.

Copy link
Member

@toyamagu-2021 toyamagu-2021 left a comment

Choose a reason for hiding this comment

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

I've doublely checked functionality.
I should note BasePage can be removed finally after both report page and workflow-list page are migrated as @agilgur5 stated.
Refs:

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.

LGTM

@terrytangyuan terrytangyuan merged commit 6fbfedf into argoproj:master Sep 21, 2023
27 checks passed
@agilgur5 agilgur5 deleted the refactor-ui-userinfo-functional branch September 21, 2023 17:24
@agilgur5
Copy link
Contributor Author

BasePage is finally gone with #11891 ! 🎉
No more 6+ year old APIs! 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants