mathesar-foundation / mathesar

Web application providing an intuitive user experience to databases.
https://mathesar.org/
GNU General Public License v3.0
2.28k stars 316 forks source link

Sort frontend imports #3552

Closed hitenvidhani closed 1 month ago

hitenvidhani commented 2 months ago

Fixes #3542

Checklist

- [x] My pull request has a descriptive title (not a vague title like `Update index.md`). - [x] My pull request targets the `develop` branch of the repository - [x] My commit messages follow [best practices][best_practices]. - [x] My code follows the established code style of the repository. - [ ] I added tests for the changes I made (if applicable). - [ ] I added or updated documentation (if applicable). - [ ] I tried running the project locally and verified that there are no visible errors. [best_practices]:https://gist.github.com/robertpainsi/b632364184e70900af4ab688decf6f53 ## Developer Certificate of Origin
Developer Certificate of Origin ``` Developer Certificate of Origin Version 1.1 Copyright (C) 2004, 2006 The Linux Foundation and its contributors. 1 Letterman Drive Suite D4700 San Francisco, CA, 94129 Everyone is permitted to copy and distribute verbatim copies of this license document, but changing it is not allowed. Developer's Certificate of Origin 1.1 By making a contribution to this project, I certify that: (a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or (b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or (c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it. (d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved. ```
hitenvidhani commented 2 months ago

@seancolsen as I was not able to find any prettier plugin which supports .svelte files and I also hopped in to the svelte community to see what people have been using and it's eslint.

I also found out that we are using eslint already in our repo so we can just extend it for checking (and fixing) sorting imports. We can either display warnings (without passing --fix) and allow the dev to fix the import themselves or we can give them an option to fix it using --fix. We can also add it to our pre-commit hooks.

The command to sort imports for a particular file(after cloning the repo and downloading the dependencies) npm run lint src/systems/data-explorer/input-sidebar/transformations-pane/TransformationsPane.svelte I have currently changed what lint executes, but we shall revert that later.

If you try to run this command without --fix(modify the package.json file), you can also see specific warnings associated to imports.

hitenvidhani commented 2 months ago

We can further modify the rules as per our requirements, but opening this PR for further discussion. Let me know if I should make it draft PR for now.

seancolsen commented 1 month ago

@hitenvidhani I added some changes to clean this up a bit.

seancolsen commented 1 month ago

@pavish I'd like you to review this.

The goal here (as described in #3542) is to set up some tooling that will keep our JS imports sorted.

You've expressed some opinions about import code style. I think that with this PR we could lean on the tooling to enforce some (but not all) of those opinions. I've also notice that import statements are the most common culprit for merge conflicts. While auto-sorting would eliminate such conflicts, I think it would reduce their frequency and complexity.

With this PR:

What I like about this:

What I don't like about this:

I was really hoping that we'd find a Prettier plugin that would work for Svelte. This one says Svelte support is coming soon. I spent some time researching and tinkering to see if we could wire up something with Prettier that would work, but I gave up. If you're not sold on this PR, then I could see us revisiting this topic in a year to see if there's a Prettier plugin that works with Svelte at that point. Using Prettier would be nice because it would format on save. I also tried getting ESLint to auto-fix on save but I wasn't able to set that up successfully.

I could go either way with this PR. Personally I'd be inclined to merge this and see how we like the new workflow. If we end up finding it annoying we could disable it. But I have a hunch that it would add a small improvement to our DX.