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

Revamp plugin logic RE #26 #73

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

Conversation

zachbryant
Copy link

@zachbryant zachbryant commented Dec 11, 2023

Firstly, thanks for taking the time to look at this. Apologies the PR is so large. When I realized my attempt at patching the swap behavior for #26 wasn't going to work, this seemed like the next best approach, but I felt it had to come with proper testing given the breadth of work required. I tried to reuse as much as I could, but the amount of edge cases required adding/replacing a lot of logic. Definitely open to feedback on this approach if anything is simpler/more sound, even if its nontrivial.

Changes included

  • Plugin sorts entire enum/interface bodies in one go as opposed to swapping nodes (Does not Sort in One Go #26)
    • Near total rewrite of plugin logic
    • New error messages including string-enum deprecation notice
    • Fix shows up for definition name & all nodes
    • Members in their correct sorted position or in locally sorted order are omitted from the error count
    • Rules are now classified as stylistic instead of warn
  • Enabled sorting all enums and deprecated string-enum (Consider sorting other types of enums? #72)
  • Revamped test suite
    • Test case options abstracted out of case data and recombined during processing
    • 100% coverage through cases + unit tests (enforced but can make more lenient)
    • Added test for definitions containing many unsorted members
    • Tests are run in random order to ensure memoization isn't bugged out

Package maintenance

  • Upgraded all deps & dev deps
  • Drop Node 16, 17 support (16 works but is EOL)
  • Added volta config
  • Added plenty of helper yarn scripts
  • Deduplicated yarn.lock with fewer strategy

Misc changes

  • Fix renamed eslint rules
  • Bump publish pipeline to Node 20
  • Add docs on deprecated rule
  • Upgrade to Rollup 4
  • Add memoization for heavy plugin computation
  • Removed default generated comments from tsconfig

Known caveats & work left

  • Deeply embedded anonymous definitions may not get sorted in one go still (thinking 10+ embedded which is very unlikely as a use case). Still need to test this and look further into it. May be acceptable for now.
  • Need to test in a real environment e.g. in another project, ideally an existing open source project making use of this plugin. Maybe I'll give it a shot in a codespace.
  • Using eslint disable comments for a line does not work. Considering adding logic to warn the user of this or to consider it in sorting.

I think this update should be released first as 4.0.0-alpha.1 in case anything goes terribly wrong.

@zachbryant
Copy link
Author

It's been a minute since I drafted this and now some packages have major bumps which change Node support. I will leave them as they are so this upgrade supports anyone who hasn't bumped to higher ESLint versions yet

@zachbryant
Copy link
Author

zachbryant commented Apr 13, 2024

Self review notes:

  • Considering making the memoization development-only. Exact same code/options seems rare which would bloat the memory usage with little time savings.
  • Need to reconcile types TSType with Node. It was necessary in-dev but also requires as unknown as Node[] to work. Unknown casting has potential to be a source of bugs.
    • Unified with NodeOrToken alias.
  • Need to test that extends: ["typescript-sort-keys/requiredFirst"] works
    • Added to exported configs.
  • Also need to tweak how the underlining works. A bit confusing for it to underline the whole enum.
    • Leaving entire enum underlined to have the quick-fix accessible without scrolling.
  • UX bug: too many things to fix
    image
    • Quick fix only added to declaration, not to individual keys/members.
  • Test generation could be refactored in the future

@zachbryant zachbryant marked this pull request as ready for review April 13, 2024 11:39
@zachbryant zachbryant marked this pull request as draft April 13, 2024 12:41
@zachbryant zachbryant marked this pull request as ready for review April 22, 2024 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant