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): use import type syntax where possible #12514

Merged
merged 1 commit into from
Feb 3, 2024

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Jan 14, 2024

Unblocks #12516
Follow-up to #12163 & #12290, which unblocked this
Follow-up to #12150 (comment)
Same changes can be made for #12158 and the argo-ui warnings introduced in #12061

Motivation

  • now that we've migrated from archived tslint to eslint and updated prettier from v1 to v3, we can finally use this newer TS syntax from TS 3.8
    • this fixes a few warnings in the build as well as makes things more explicit and efficient
      • note: there are more warnings to fix around export type, but those are all from argo-ui's source code and so can't be fixed from within this repo

Modifications

  • use import type syntax in a few files where it is only types that are imported

Verification

  1. yarn build works fine, with a few less warnings now
  2. make start UI=true and tested UI, esp. monaco-editor -- all still works fine

- now that we've migrated from archived `tslint` to `eslint` and updated `prettier` from v1 to v3, we can _finally_ use this newer TS syntax
  - this fixes a few warnings in the build as well as makes things more explicit and efficient
    - note: there are more warnings to fix around `export type`, but those are all from `argo-ui` and so can't be fixed from within this repo

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

Great! LGTM

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.

Why is this a fix?

@agilgur5
Copy link
Contributor Author

agilgur5 commented Jan 14, 2024

Why is this a fix?

because it fixes existing warnings. those warnings also become errors in #12516

could potentially word it as a build commit, although it doesn't directly touch the build, the changes are to the source code.

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

@agilgur5 agilgur5 changed the title fix(ui): use import type syntax where possible refactor(ui): use import type syntax where possible Jan 14, 2024
@agilgur5
Copy link
Contributor Author

could potentially word it as a build commit, although it doesn't directly touch the build, the changes are to the source code.

I re-worded it as a refactor so that it's not cherry-picked into patches since it relies on other build changes (#12163 and #12290)

@juliev0
Copy link
Contributor

juliev0 commented Jan 26, 2024

How would you feel about being assignee on this @terrytangyuan ? Given that it's a "prioritized-review", and has been for over a week, it needs an Assignee. I don't think I have the right background for this. But I see it already has 2 approvals. It also looks like this other prioritized-review PR is ready but blocked on this one.

@terrytangyuan
Copy link
Member

I have a concern on these small refactor PRs. Started discussion on Slack https://cloud-native.slack.com/archives/C0510EUH90V/p1706279158785129

@agilgur5
Copy link
Contributor Author

agilgur5 commented Jan 26, 2024

Started discussion on Slack https://cloud-native.slack.com/archives/C0510EUH90V/p1706279158785129

Splitting out a new repo is a future-facing question, that historically has taken months+ to occur (the SDKs being a current example of that) and requires org-level permissions that few people have.
Meanwhile this PR has already been prioritized for 2 weeks, surpassing the sustainability effort requirements (notably, I have other PRs that I did not prioritize out for longer and I've also been out sick).

A new repo requires even more time and will require splitting the git history, re-creating PRs, re-approving PRs, and then, once again, waiting for a merge from an Approver+....

@juliev0
Copy link
Contributor

juliev0 commented Feb 3, 2024

@isubasinghe Are you comfortable merging this since you've given approval?

@isubasinghe
Copy link
Member

@juliev0 I am comfortable merging this, but I don't have write access yet I think?

@juliev0
Copy link
Contributor

juliev0 commented Feb 3, 2024 via email

@terrytangyuan terrytangyuan merged commit 13444e6 into argoproj:main Feb 3, 2024
20 checks passed
@agilgur5 agilgur5 deleted the fix-ui-import-type branch February 3, 2024 17:35
@agilgur5
Copy link
Contributor Author

agilgur5 commented Feb 3, 2024

@juliev0 I am comfortable merging this, but I don't have write access yet I think?

Got it! Okay, should be soon.

Crenshaw said he'd get to membership etc updates on Monday, for reference

@terrytangyuan
Copy link
Member

I’ll add once it’s publicly announced

isubasinghe pushed a commit to isubasinghe/argo-workflows that referenced this pull request Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui prioritized-review For members of the Sustainability Effort
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants