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): flatten directory structures #12539

Merged
merged 21 commits into from
Feb 20, 2024

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Jan 17, 2024

Motivation

Remove lots of unnecessary nesting

  • a folder for a single file or two tends to make things harder to read
    • lots of these were just a single file, where having a directory is redundant
    • some were a TSX file plus an SCSS file, which will be right next to each other in editors as files are sorted alphabetically, so this is not necessary for just two files. some editors will also have icons for different filetypes that make these easy to distinguish as well
    • some were a single file plus a single directory
      • usually an index.ts plus a components/ directory, which sometimes itself only had 1 file or 1 directory in it, so lots of redundant folders
      • some of the nesting was also inconsistent, with non-component util files under components/ and some directories not having a components/ dir etc
  • it required a lot of clicks or cds to get anywhere in the UI as there's so much directory nesting without any need for it
    • for instance, the plugin-list component took 3 clicks to get to, even though it's the only real component under plugins/ (plus a routing wrapper)

Have wanted to do this for a while, but it took a solid amount of time and has a sizeable diff. This finally tackles a big chunk of it while there aren't too many other UI changes out to merge conflict

Modifications

Flatten several nested directories

These had several directories under them, now all the files are just under the directory listed without redundant nesting

  • api-docs/
  • cron-workflows/
  • cluster-workflow-templates/
  • event-flow/
  • event-sources/
  • help/
  • login/
  • modals/
  • plugins/
  • reports/
  • sensors/
  • shared/components/
  • userinfo/
  • workflow-event-bindings/
  • workflow-templates/

Move shared hooks into one directory in shared/ dir

  • most were in shared/, but one was in shared/components/ (despite not being a component), and another was created in a separate shared/hooks/ dir
    • put them all in shared/ for consistency right now
      • might be good to put them into shared/hooks/, but for now make it consistent at least
        • also might not be necessary since they do all start with use- by convention
    • fix naming of use-local-storage which previously did not use any casing, but should use kebab-case for consistency with the rest of the UI codebase

Other misc changes along the way

  • rename a few index.tsx to index.ts

    • as they have no JSX, same as the other index.ts files in the codebase
    • plus a few other files like version.tsx -> version.ts etc
      • rewrite this file to a readable function instead of a large one-liner as well
      • plus remove deprecated / non-spec substr function and replace with substring
  • rename to api-docs from apiDocs for consistency with the rest of UI codebase

    • i.e. kebab-case file and directory names, not camelCase
    • this isn't a strict JS convention, but is the most commonly used one
      • (whereas there are conventions for classnames, module names, etc)
  • use named functions instead of consts assigned to anonymous functions for better debug tracing

Verification

yarn build passes

Future Work

  1. workflows/ and shared/ still need some more organizing

    • both of these are quite large with many files, so flattening wouldn't be the primary approach here. we probably want directories in a logical way
      • for workflows/, maybe split by pages, the WorkflowDetails page and the WorkflowsList page
      • for shared/, it is currently internally inconsistent as it has a components/ dir and then also components outside of that dir. I made it more consistent, but there's still more to do there.
  2. src/app/ also has some unnecessary nesting

  3. Add a linter plugin to sort import statements (similar to isort like Python) so I don't keep editing these manually whenever I see them not meeting conventions

appearance: none;
}
}
}
Copy link
Contributor Author

@agilgur5 agilgur5 Jan 17, 2024

Choose a reason for hiding this comment

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

this file got automatically "modified" because it had inconsistent line endings (i.e. LF vs CRLF). otherwise there are no content changes

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.

Tested and works as expected, I don't have any strong opinions but can confirm it is easier to navigate now.

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.

One concern is that it would be hard to cherry pick UI changes into patch releases.

@agilgur5
Copy link
Contributor Author

agilgur5 commented Jan 19, 2024

One concern is that it would be hard to cherry pick UI changes into patch releases.

Git picks up on renames, so a change made to a renamed file will be properly cherry-picked into its earlier location.

So the only real difficulties could be when there is a merge conflict with the code changes here -- mostly the import paths -- but a cherry-picked bugfix is unlikely to change an import path, so there's a pretty low chance of conflict as well. If a new file were added, that could make things confusing, but similarly, that is unlikely to happen in a cherry-picked bugfix.
And if all else fails, we could cherry-pick this in as well if we wanted to. So I think that concern has low probability and is relatively straightforward to overcome as well.

Also in general, I don't think we should limit improving tech debt based on what can be cherry-picked, although that is a good consideration to keep in mind.
Notably, linter upgrades such as #12163 / #12290 have significantly higher chances of causing issues with cherry-picking, but we do still make those upgrades. And any actual semantic refactor, style change, or feature also has significantly higher chance of causing issues with cherry-picking too, since the contents actually change -- this change / renames in general are actually one of the lowest chance of causing issues in comparison.

- remove some unnecessary nesting
  - a folder for one file just makes things harder to read
  - and there are no other folders either

- rename `index.tsx` to `index.ts`
  - as it has no JSX, same as the other `index.ts` files in the codebase

- rename to `api-docs` from `apiDocs` for consistency with the rest of UI codebase
  - i.e. kebab-case file and directory names, not camelCase
  - this isn't a strict JS convention, but is the most commonly used one
    - (whereas there are conventions for classnames, module names, etc)

- use named functions instead of consts assigned to anon functions for better tracing

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- some components were in the `components/` dir and others were nested
  - a folder for a single file is unnecessary and makes reading harder
  - a folder for two files, an SCSS file and the TSX file, is not much better either
    - they appear right next to each other in editors as they're alphabetized by filename anyway, and the file ending is last
    - and in some editors/themes, are clearly distinguished by icons as well

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- after previous flattening, it was one `index.ts` file and then a `components/` directory with everything else
  - just flatten those into one, simplifying reading with less nesting

- also partially sort imports (similar to isort) by putting a newline between external deps and internal deps
  - TODO: add an `eslint` plugin for this now that we're on `eslint` / off `tslint`

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- `components/` had one file then a directory with other files
  - the `event-flow-details/` dir with several closely related files makes sense as an abstraction
  - but the `components/` dir itself with one file does not and is harder to read

- add new line between external and internal imports

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- for same reasons as previous coommits

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- I thought the `event-flow-details/` dir might make sense as an abstraction, but then I realized:
  1. `id.ts`, for instance, is also used in `event-sources/` and `sensors`, so not exactly abstracted inside then
  2. it's the only real component in `event-flow/`, since the container is just a routing wrapper

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- both have a single component

- also rename `index.tsx` to `index.ts` since it has no JSX

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- `feedback` and `first-time-user` literally had a single file in them, so redundant dirs
- `new-version` had only two files in it

- interestingly, no `components/` dir here, so this is one example of where the previous directory structure was not internally consistent
  - there's a few others of those in the UI codebase

- rename `version.tsx` to `version.ts` as it has no JSX
  - rewrite it to a readable function instead of a large one-liner
  - remove deprecated / non-spec [`substr`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/substr) function and replace with `substring`

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- `plugins-list/` dir had exactly one file in it; redundant dir
- `components/` only had 1 real component, `plugin-list`, plus its wrapper routing container
  - 2 files for a folder not really worth and you had to go 3 folders in to get to the one actual component

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- same as previous commits
- notably `workflows-to-chart-data.ts` isn't a component, it's a util module

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- mostly same reasons as previous commits
- `sensor-details` and `sensor-list` had one file per folder, which is redundant
- notably, `utils.ts` is not a component and was used outside of `sensors` too

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- same as previous commits

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- same as previous commits
- couple redundant folders here with exactly 1 file in them

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- same as previous commits
- waited for a bit before tackling this dir as it has more components than others
  - but still only around ~10, so not _that_ many

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- most were in `shared/`, but one was in `components/` (despite not being a component), and another was created in a separate `hooks/` dir
  - put them all in `shared/` for consistency right now
    - might be good to put them into `shared/hooks/`, but for now make it consistent at least
      - also might not be necessary since they do all start with `use-` by convention
  - fix naming of `use-local-storage` which previously did not use any casing, but should use kebab-case for consistency with the rest of the UI codebase

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- this provides better tracing in source maps and when debugging etc
- got through most of these in previous PRs, but `shared/` still had a bunch

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- whenever there was a redundant folder around a single component, flatten the directory structure

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- where there was only 1 or 2 files per folder, flatten them
- the rest I need to take a harder look at
- and this directory _does_ have too many files in right now
  - I would _probably_ split this into directories by the WorkflowDetails page and the WorkflowsList page
    - i.e. the same way it is split visually and in the routing, for intuitivity

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
This reverts commit f881199.

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- some things my editor seems to have missed, these failed on build, should pass now
  - mostly SCSS files, probably because TS (which powers VS Code) doesn't recognize those natively (Webpack handles them in the build)

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5
Copy link
Contributor Author

agilgur5 commented Feb 20, 2024

but a cherry-picked bugfix is unlikely to change an import path

Fixed a few conflicts, and notably this still stands over a month later, the conflicts were all with features (#12616, #12674) that won't be backported.

I'd like to get this in as it does help with navigating through all the directories quite a bit (it literally makes me more productive). Since that has been approved and I think the one comment has been addressed here -- this doesn't really impact backports as per above -- and on Slack via the release rotation #12592, I'm going to merge this

@agilgur5 agilgur5 merged commit d3433c6 into argoproj:main Feb 20, 2024
15 checks passed
@agilgur5 agilgur5 deleted the refactor-ui-reorganize-directories branch February 20, 2024 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants