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

feat: port all order rule new options from upstream #248

Merged
merged 3 commits into from
Mar 18, 2025

Conversation

TrevorBurnham
Copy link

@TrevorBurnham TrevorBurnham commented Mar 17, 2025

This PR includes all order rule new options:

There is a special case working different from upstream with same codes, so I have to add an internal check at https://github.com/un-ts/eslint-plugin-import-x/pull/248/files#diff-751be091169bc714939b692c4def0f6e915f2a7f675611dc1e025cd19e6bb575R437-R449 to suppress the false positive reports/failing test cases.

close #234
close #225


original PR

This PR adds support for sorting named imports in the order rule. This functionality was added to eslint-plugin-import by import-js#3043.

Resolves #225

Notes

This was an experiment for me in using Claude Sonnet 3.7 to add a feature to a complex, unfamiliar codebase. I had it copy the test cases from the eslint-plugin-import patch, but did not use the eslint-plugin-import code as a reference, since I wanted it to adhere to the standards established in the eslint-plugin-import-x codebase.

The results of the experiment were… mixed. Claude generated a mostly-working solution right away, but its code was a sprawling mess. Worse, when I pointed out test failures, it had a habit of adding special cases to "fix" the problem. 🤦🏻‍♂️ What you have here is the result of me iterating with Claude many, many times, making manual tweaks along the way. I also refactored much of the code into utility modules to keep order.ts sane.

One takeaway for me here is that this feature is much more complicated than I'd expected. Implicit requirements from the test cases like "comments should move with the named import they're attached to" required a lot of code to handle. In particular, about half the code is dedicated to handling CommonJS. Claude generated completely different codepaths for the CommonJS cases, and while I'm sure those could be combined more elegantly with the ESM cases, it'd take a substantial effort.

So my question to the maintainers is: Should CommonJS be supported here? eslint-plugin-import-x's resume suggests that it's for ESM only:

This plugin intends to support linting of ES2015+ (ES6+) import/export syntax…

But the order rule docs say:

Enforce a convention in the order of require() / import statements.

If you want to drop support for CommonJS here, I'd be happy to make another revision with cleaner code.

Copy link

changeset-bot bot commented Mar 17, 2025

🦋 Changeset detected

Latest commit: b27ef4c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
eslint-plugin-import-x Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

codesandbox-ci bot commented Mar 17, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link
Member

@JounQin JounQin left a comment

Choose a reason for hiding this comment

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

Thanks for your great work first! I'm on mobile right now, and will take a deep look tomorrow.

Could you please fix the conflicts first?

@JounQin
Copy link
Member

JounQin commented Mar 17, 2025

commonjs is still supported.

Copy link

codecov bot commented Mar 17, 2025

Codecov Report

Attention: Patch coverage is 98.11321% with 7 lines in your changes missing coverage. Please review.

Project coverage is 96.17%. Comparing base (00d486c) to head (b27ef4c).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/rules/order.ts 98.00% 6 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #248      +/-   ##
==========================================
+ Coverage   96.08%   96.17%   +0.09%     
==========================================
  Files         107      107              
  Lines        4799     5052     +253     
  Branches     1624     1786     +162     
==========================================
+ Hits         4611     4859     +248     
+ Misses        188      187       -1     
- Partials        0        6       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@TrevorBurnham
Copy link
Author

TrevorBurnham commented Mar 17, 2025

@JounQin I've rebased my changes against master and made the updates you called out. Thanks for the quick feedback!

@JounQin
Copy link
Member

JounQin commented Mar 17, 2025

There are lines of codes uncovered in the tests.

@JounQin
Copy link
Member

JounQin commented Mar 17, 2025

I'm going to sleep now. I'll take another review tomorrow.

@JounQin JounQin marked this pull request as draft March 18, 2025 14:37
@JounQin
Copy link
Member

JounQin commented Mar 18, 2025

I'll do 1:1 port instead for maintainance reason.

@JounQin JounQin force-pushed the named-import-order branch from 97a5b17 to 3ff7b10 Compare March 18, 2025 17:05
Copy link

pkg-pr-new bot commented Mar 18, 2025

Open in Stackblitz

npm i https://pkg.pr.new/un-ts/eslint-plugin-import-x@248

commit: b27ef4c

@JounQin JounQin force-pushed the named-import-order branch from 3ff7b10 to 3c6f9a6 Compare March 18, 2025 20:49
@JounQin JounQin force-pushed the named-import-order branch from 3c6f9a6 to 01b4c75 Compare March 18, 2025 21:35
@JounQin JounQin changed the title feat: add named import sort option to order rule feat: migrate all new order options Mar 18, 2025
@JounQin JounQin changed the title feat: migrate all new order options feat: migrate all order rule new options Mar 18, 2025
@JounQin JounQin changed the title feat: migrate all order rule new options feat: port all order rule new options from upstream Mar 18, 2025
@JounQin JounQin marked this pull request as ready for review March 18, 2025 22:05
@JounQin JounQin enabled auto-merge (squash) March 18, 2025 22:09
@JounQin JounQin disabled auto-merge March 18, 2025 22:12
@JounQin JounQin merged commit 6940b14 into un-ts:master Mar 18, 2025
37 checks passed
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.

Support for newlines-between-types? import/order sorting specifiers, named imports
2 participants