eslint-plugin-typescript-sort-keys icon indicating copy to clipboard operation
eslint-plugin-typescript-sort-keys copied to clipboard

Revamp plugin logic RE #26

Open zachbryant opened this issue 1 year ago • 2 comments

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 (#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 (#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.
  • [x] 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 avatar Dec 11 '23 09:12 zachbryant

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 avatar Apr 13 '24 07:04 zachbryant

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.
  • [x] 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.
  • [x] Need to test that extends: ["typescript-sort-keys/requiredFirst"] works
    • Added to exported configs.
  • [x] 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.
  • [x] 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 avatar Apr 13 '24 09:04 zachbryant