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

Sort frontend imports #3552

Merged
merged 11 commits into from May 7, 2024
Merged

Conversation

hitenvidhani
Copy link
Contributor

@hitenvidhani hitenvidhani commented Apr 20, 2024

Fixes #3542

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the develop branch of the repository
  • My commit messages follow best practices.
  • 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.

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
Copy link
Contributor Author

hitenvidhani commented Apr 20, 2024

@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 hitenvidhani marked this pull request as ready for review April 20, 2024 19:13
@hitenvidhani
Copy link
Contributor Author

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 seancolsen self-assigned this Apr 22, 2024
@seancolsen seancolsen added the pr-status: review A PR awaiting review label Apr 22, 2024
@seancolsen seancolsen changed the title Draft - sort frontend imports Sort frontend imports Apr 29, 2024
@seancolsen
Copy link
Contributor

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

@seancolsen seancolsen assigned pavish and unassigned seancolsen May 7, 2024
@seancolsen
Copy link
Contributor

@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:

  • We get more linting errors when imports are out of order. This prevents messy code from getting merged by catching it in CI.
  • We can auto-sort imports by running eslint with --fix (also now exposed via npm run lint-fix).

What I like about this:

  • Less worrying about import syntax while coding

What I don't like about this:

  • Unless we remember to run the lint-fix command, we're more likely to get tripped up by CI errors.
  • Manually running ESLint is a drag because ESLint is slow. It takes about 80 seconds for me. That's not terrible, but it is annoying. I don't run ESLint manually very often. This PR would require me to do this more often.
  • I noticed what appears to be a bug in one of these ESLint rules — under certain circumstances (which I don't fully understand) ESLint adds an extra blank line after all the import statements. This happened for roughly 1% of the files that I auto-formatted in this PR. That extra blank line is a line that Prettier strips out. Annoyingly, this means that sometimes running eslint --fix is not enough — we also need to run prettier --write thereafter. Perhaps this is rare enough to be only a minor nuisance though.

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.

Copy link
Member

@pavish pavish left a comment

Choose a reason for hiding this comment

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

@hitenvidhani @seancolsen This looks good to me!

Sean, I agree it would be a bit annoying to deal with having to run eslint regularly, and running both eslint and prettier in case of the additional line. I do think the DX would be worth it, and we can always remove this if it's not, as you mentioned.

@pavish pavish added this pull request to the merge queue May 7, 2024
Merged via the queue into mathesar-foundation:develop with commit 4e619c6 May 7, 2024
36 checks passed
@hitenvidhani hitenvidhani deleted the sort_fe_imports branch May 11, 2024 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: review A PR awaiting review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto sort frontend imports
3 participants