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

Code formatting #51

Open
sirbully opened this issue Jul 6, 2024 · 12 comments
Open

Code formatting #51

sirbully opened this issue Jul 6, 2024 · 12 comments
Assignees

Comments

@sirbully
Copy link
Contributor

sirbully commented Jul 6, 2024

I notice while working on the coverage issue that we haven't streamlined formatting our code. I'd like to recommend Prettier 🙌 It would also be nice to enforce and automatically handle sort order of our imports.

@sirbully
Copy link
Contributor Author

sirbully commented Jul 6, 2024

@ann-kilzer would like to hear your opinion since both of us will be working on this project a lot so let me know if you have preferences 👀 Also, if it's alright with you, I'd like to pick this up 🤚

@ann-kilzer
Copy link
Collaborator

Since eslint has added formatting and stylistic rules, I don't like using prettier. It's another tool to manage and complicates the build steps.

We have some formatting in place. Are there any gaps you notice?

@ann-kilzer ann-kilzer mentioned this issue Jul 6, 2024
@sirbully
Copy link
Contributor Author

sirbully commented Jul 6, 2024

I format on save which adds semicolons orz I don't think eslint have any power over it (or it doesn't care). While coding, all my single quotes also changed to double quotes and I had to manually change them back to single 🙈 (Nvm, the single quote one is resolved) Not the best dev experience 😭

@sirbully
Copy link
Contributor Author

sirbully commented Jul 6, 2024

Sample of my dev experience, notice it adds semicolons on line 1 and 2

Screen.Recording.2024-07-06.at.22.36.27.mov

@ann-kilzer
Copy link
Collaborator

We may have different people with different developer environments working on the project, and so we have to find a way to align our tooling. I like EditorConfig for a lot of this, and I'll update the eslint rules to have a semi rule. I can look into import order too

The video you share above may be a result of the Prettier plugin. You should disable for this workspace: https://stackoverflow.com/a/75471109/1860768

@sirbully
Copy link
Contributor Author

The eslint plugins we use at work for import sort order are:

Just sharing for reference! Those two work really well

@ann-kilzer
Copy link
Collaborator

@sirbully Thanks for sharing! 🤩

I will have to take more time to look at perfectionist in detail.

simple-import-sort looks interesting and straightforward. If we want something more configurable, I notice https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/order.md . What do you think?

One note with MUI is we should encourage full imports import Component from '@mui/material/Component rather than import { Component } from '@mui/material' (this keeps the bundle smaller) so hopefully whatever import sorting tool aligns with that

@sirbully
Copy link
Contributor Author

sirbully commented Jul 16, 2024

Hey @ann-kilzer, did a little more digging into this.

We moved away from eslint-plugin-import since it didn't support eslint v9 (only up to v8) and typescript-parser v7 (only up to v5). Checking our dependencies, it looks like we use eslint v8 and typescript-parser v7 so eslint-plugin-import will be incompatible with our dev setup.

Upon checking, perfectionist should be good. Apart from satisfying dependency version requirements, it only has minimal dependencies (3) in order for the plugin to work so it won't bloat our project during development.

I also checked simple-import-sort and it only supports typescript-parser v6 so let's not use that 🙇‍♀️

Perfectionist should be super easy to setup! We just need to decide how we want to organize or imports (and exports if you care about that, but from what I noticed we usually export default, so we don't have to setup sorting for exports). No worries about MUI since the importing plugin only checks after the from of the import statement and starts sorting from there. Checking what is imported (full import or otherwise) is out of scope for the sorter.

@sirbully
Copy link
Contributor Author

sirbully commented Jul 16, 2024

Sample setup, docs here: https://eslint-plugin-perfectionist.azat.io/rules/sort-imports#sort-imports

  • Trigger error if imports are incorrectly sorted
  • type specifies how imports are sorted, default: alphabetical
  • By default, for every group of imports, a newline will be added (behavior can be changed)
  • Sorting:
    • Any import with react in its name (e.g. react, react-dom) will be first (custom group)
    • Followed by builtin (e.g. Node.js modules like path) and external (other non-react installed modules like MUI) modules
    • Followed by internal modules (components like @/Header)
    • Followed by relative imports (../FileName, ./FileName)
    • Finally stylesheets
"perfectionist/sort-imports": [
  "error",
  "type": "alphabetical", // optional, default value is alphabetical, can remove this
  "newlines-between": "always", // optional, default value is always, can remove this
  {
    "groups": [
      "react",
      ["builtin", "external"],
      "internal",
      ["parent", "sibling"],
      "style",
    ],
    "custom-groups": {
      "value": {
        "react": ["react", "react-*"]
      }
    }
  }
],

@ann-kilzer
Copy link
Collaborator

@sirbully Interesting, thanks for looking into it. It looks like it might be helpful in a larger software project, at the same time, I'd like to keep this codebase lean in terms of features and maintenance. I notice eslint has a built-in import sorting rule: https://eslint.org/docs/latest/rules/sort-imports

Would you think this rule would suffice? Or do you find the grouping to be helpful?

@sirbully
Copy link
Contributor Author

@ann-kilzer I personally prefer the grouped sorting as it helps with code readability and I feel that a clear import pattern as early as now contributes to the maintainability of our project in the long term.

We will only need to set this up once so it won't have a lot of overhead and will be as lean as possible 🙇‍♀️

@ann-kilzer
Copy link
Collaborator

Cool, let's give it a try then

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

No branches or pull requests

2 participants