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

BED-5112: use prettier to organize imports #1026

Merged
merged 18 commits into from
Jan 15, 2025
Merged

BED-5112: use prettier to organize imports #1026

merged 18 commits into from
Jan 15, 2025

Conversation

benwaples
Copy link
Contributor

Description

Adds a plugin to prettier that sorts, combines and removes unused imports using the organizeImports feature of the TS language server.

Motivation and Context

This PR addresses: BED-5112

Organized imports helps devs parse where code is coming from. This is particularly helpful as we replace MUI with DoodleUI and many of the components will have similar, if not the same, names to help migrate to the new component library.

Additionally, this plugin will remove unused imports, so one less thing to review for.

The npm documentation is very helpful, feel free to give it a review!

How Has This Been Tested?

confirmed that prettier will automatically update imports.

Screenshots (optional):

n/a

Types of changes

  • Chore (a change that does not modify the application functionality)

Checklist:

@benwaples benwaples self-assigned this Dec 18, 2024
@benwaples benwaples added enhancement New feature or request user interface A pull request containing changes affecting the UI code. labels Dec 18, 2024
Copy link
Contributor

@mistahj67 mistahj67 left a comment

Choose a reason for hiding this comment

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

LGTM, tested locally did in fact organize my imports automagically 🪄

Copy link
Contributor

@urangel urangel left a comment

Choose a reason for hiding this comment

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

🚀

@elikmiller
Copy link
Contributor

I like the idea but I am seeing one issue.

After updating my dependencies and running yarn format in cmd/ui, packages/javascript/js-client-library and packages/javascript/bh-shared/ui I am no longer able to build the UI project and the following error appears.

src/main.tsx:33:16 - error TS2664: Invalid module name in augmentation, module '@mui/styles/defaultTheme' cannot be found.

33 declare module '@mui/styles/defaultTheme' {
                  ~~~~~~~~~~~~~~~~~~~~~~~~~~


Found 1 error in src/main.tsx:33

Copy link
Contributor

@elikmiller elikmiller left a comment

Choose a reason for hiding this comment

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

Please see my above comment!

@benwaples
Copy link
Contributor Author

@elikmiller thats sus! Ill check it out. Thanks!

@benwaples benwaples changed the title feat: use prettier to organize imports BED-5112: use prettier to organize imports Jan 8, 2025
@benwaples
Copy link
Contributor Author

@elikmiller found the problem! But its super weird. Its not directly related to this prettier plugin, but a side-effect from formatting the imports.

In cmd/ui/src/App.test.tsx, Importing bh-shared-ui before the src/App component caused this error. Im not entirely sure why yet, but the fix is to essentially organize the App files under an App directory such as:

src/
  App/
    App.tsx
    App.test.tsx
    index.ts
  main.ts // imports App

@elikmiller
Copy link
Contributor

elikmiller commented Jan 8, 2025

@benwaples It looks like if we delete this module declaration in cmd/ui/src/main.tsx it also works.

I don't exactly remember why we needed this declaration in the first place, might have been a missed cleanup when we moved from MUI 4 to 5.

- declare module '@mui/styles/defaultTheme' {
-     interface DefaultTheme extends Theme {}
- }

@benwaples
Copy link
Contributor Author

@elikmiller from what I can tell it was part of the migration from 4 -> 5 so I was hesitant to delete it. Sounds like their reasoning for the augmentation is if we use makeStyles or withStyles (which we do), but deleting the augmentation doesnt cause TS or build errors...

If TS and the build is happy, Im happy to remove. Are you still comfortable with that?

@elikmiller
Copy link
Contributor

@benwaples Yes and it looks like we can remove it from bh-shared-ui too. I'd suggest those changes as well as checking in all the files after running yarn format in each project (which I know will be a huge change)

@benwaples
Copy link
Contributor Author

@elikmiller discussed offline, but to summarize, we are keeping the module augmentation in bh-shared-ui and believe there was a collision in module scope when loading the bh-shared-ui package before having the chance to define the augmentation in bhce.

@benwaples
Copy link
Contributor Author

@elikmiller came across more import ordering errors in cmd/ui/src/setupTests.tsx, packages/javascript/bh-shared-ui/src/test-utils.jsx, and packages/javascript/bh-shared-ui/src/setupTests.tsx. It appears, that the formatting is removing unused variables that are imported purposefully for either mocked or scoping reasons.

@benwaples benwaples merged commit 7f7b17f into main Jan 15, 2025
5 checks passed
@benwaples benwaples deleted the BED-5112 branch January 15, 2025 00:01
@github-actions github-actions bot locked and limited conversation to collaborators Jan 15, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request user interface A pull request containing changes affecting the UI code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants