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(ts): refactor types into separate modules #1107

Merged
merged 3 commits into from
Sep 29, 2023

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Sep 8, 2023

Welcome to Cryostat! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed all commits using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main


Fixes: #941

Description of the change:

Move common types into types.ts to avoid cirular deps. Also, fix some type annotations that eslint might have missed.

Motivation for the change:

See #941

@tthvo tthvo added chore Refactor, rename, cleanup, etc. safe-to-test labels Sep 8, 2023
@tthvo tthvo marked this pull request as draft September 8, 2023 07:31
@tthvo tthvo force-pushed the circular-deps branch 2 times, most recently from 18e64ae to 1ea6e23 Compare September 13, 2023 08:39
@tthvo tthvo force-pushed the circular-deps branch 3 times, most recently from d5dab4a to 3b538f0 Compare September 24, 2023 08:04
@tthvo
Copy link
Member Author

tthvo commented Sep 25, 2023

@andrewazores I just realized this is quite a huge set of changes. Do you think we have time to review for 2.4?

If not, I think we could delay this till next release and use it as a chance to go over web UI functionalities from top to toe?

@tthvo
Copy link
Member Author

tthvo commented Sep 25, 2023

Though, rebasing will be painful haha I will try to keep up with other PRs.

@andrewazores
Copy link
Member

I would rather try to get this in ASAP since it is so large and rebasing it will only get harder. It may not make it in time for the 2.4 release but that's fine, it can be merged into main after the release window as soon as it is ready as one of the first merges into the next development cycle.

@tthvo
Copy link
Member Author

tthvo commented Sep 25, 2023

Suree just some little moving around and lots of tests broken left. Will try to get it done asap.

@tthvo tthvo marked this pull request as ready for review September 26, 2023 05:47
@tthvo
Copy link
Member Author

tthvo commented Sep 26, 2023

Ready now ^^ Most changes are just moving definitions into new files. The following are the conventions:

  • .types.ts(x): Define type definitions. This includes types and enums.
  • .utils.ts(x): Define utility functions. Also, might contain constants (usually tightly coupled with the utility functions).
  • .const.ts: Define constants. These are mostly constants that are purely for UI rendering.
  • context.tsx: Define React contexts. Mostly, to resolve circular deps. These can be defined in utils.ts(x).

I tried to move definitions closed to where they are used (colocation?). Quite a huge change but I guess its a good chance to go over the web UI as a whole.

@andrewazores
Copy link
Member

Ready now ^^ Most changes are just moving definitions into new files. The following are the conventions:

* `.types.ts(x)`: Define type definitions. This includes types and enums.

* `.utils.ts(x)`: Define utility functions. Also, might contain constants (usually tightly coupled with the utility functions).

* `.const.ts`: Define constants. These are mostly constants that are purely for UI rendering.

* `context.tsx`: Define React contexts. Mostly, to resolve circular deps. These can be defined in `utils.ts(x)`.

I tried to move definitions closed to where they are used (colocation?). Quite a huge change but I guess its a good chance to go over the web UI as a whole.

Document these conventions in README or a CONTRIBUTING doc please :-)

@andrewazores
Copy link
Member

/build_test

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1107-18d6eeab5cfe28c58ec81fd4cb16156d7cc88b84 sh smoketest.sh

@andrewazores
Copy link
Member

Sorry @tthvo ... this needs a big rebase after #1117

@tthvo
Copy link
Member Author

tthvo commented Sep 28, 2023

Ah right! I will go over it by this week!

@tthvo
Copy link
Member Author

tthvo commented Sep 29, 2023

/build_test

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1107-5f69a43d78994f6166806d060cedb3c1aa05ead7 sh smoketest.sh

@tthvo
Copy link
Member Author

tthvo commented Sep 29, 2023

Phew, finished rebasing :)) Also, added a small padding for recording option label group.

Before After
before after

@tthvo
Copy link
Member Author

tthvo commented Sep 29, 2023

/build_test

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1107-92310953b44826ebed9cd1aeee288c125da1f46e sh smoketest.sh

Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

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

Awesome work, thanks @tthvo

@andrewazores
Copy link
Member

I do see one last dependency cycle:

$ yarn run lint
[format] [prettier:check] Checking formatting...
[format] [license:check] No default format specified. Using {"prepend":"/*","append":"*/"} as backup
[format] [license:check] ✔ All files have licenses.
[format] [license:check] Command succeeded
[format] [license:check] yarn run license:check exited with code 0
[format] [prettier:check] All matched files use Prettier code style!
[format] [prettier:check] yarn run prettier:check exited with code 0
[format] yarn run format exited with code 0
[type-check] yarn run type-check exited with code 0
[eslint] 
[eslint] /home/work/workspace/cryostat-web/src/app/Shared/Redux/ReduxStore.tsx
[eslint]   23:1  warning  Dependency cycle detected  import/no-cycle
[eslint] 
[eslint] ✖ 1 problem (0 errors, 1 warning)
[eslint] 
[eslint] yarn run eslint exited with code 0

@tthvo
Copy link
Member Author

tthvo commented Sep 29, 2023

Hmm, not seeing it on my end tho. Any idea?

$ yarn lint
[eslint] yarn run eslint exited with code 0
[format] [prettier:check] Checking formatting...
[format] [license:check] No default format specified. Using {"prepend":"/*","append":"*/"} as backup
[format] [license:check] ✔ All files have licenses.
[format] [license:check] Command succeeded
[format] [license:check] yarn run license:check exited with code 0
[format] [prettier:check] All matched files use Prettier code style!
[format] [prettier:check] yarn run prettier:check exited with code 0
[format] yarn run format exited with code 0
[type-check] yarn run type-check exited with code 0

@tthvo
Copy link
Member Author

tthvo commented Sep 29, 2023

Just double checked. That should be fixed with import type { RootState } from '../ReduxStore'; within src/app/Shared/Redux/Middlewares/PersistMiddleware.tsx

There is no complaints with github either?

image

@andrewazores
Copy link
Member

Weird. Good enough for me - I'll figure out what's happening on my setup then.

@andrewazores andrewazores merged commit 337db94 into cryostatio:main Sep 29, 2023
@tthvo tthvo deleted the circular-deps branch September 29, 2023 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Refactor, rename, cleanup, etc. safe-to-test
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Task] Remove circular dependencies from code base.
2 participants