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

[order] Sort type imports #2398

Merged
merged 1 commit into from
Sep 7, 2022

Conversation

aaronadamsCA
Copy link
Contributor

Here's a preliminary PR that resolves #2339. It's my first contribution, so please bear with me! 🙂

  • Type imports are now sorted before regular imports, because this appears to be the overwhelming ecosystem standard.
  • To reduce ambiguity, error messages can now refer to "type import" instead of just "import".
  • I included typeof support just in case (though I don't know much about Flow).
  • Tests were updated to reflect revised behaviour, and two new tests were added.

I also did some opportunistic code cleanup:

  • Some test messages seemed to reflect old non-deterministic rule behaviour with "before" vs. "after" errors. Given this is now predictable, I made the tests easier to understand and maintain.
  • Coding style toward the end of the test file was all over the place, so I cleaned it up.
  • I made a minor performance improvement in fixOutOfOrder.

Let me know if there's anything I should undo here, or separate into separate PRs.

…ts with same path based on their kind (`type`, `typeof`)

Fixes import-js#2339

Co-authored-by: Aaron Adams <[email protected]>
Co-authored-by: stropho <[email protected]>
@codecov
Copy link

codecov bot commented Mar 7, 2022

Codecov Report

Base: 95.16% // Head: 94.98% // Decreases project coverage by -0.17% ⚠️

Coverage data is based on head (cff27b1) compared to base (395e26b).
Patch coverage: 92.00% of modified lines in pull request are covered.

❗ Current head cff27b1 differs from pull request most recent head c4f3cc4. Consider uploading reports for the commit c4f3cc4 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2398      +/-   ##
==========================================
- Coverage   95.16%   94.98%   -0.18%     
==========================================
  Files          66       66              
  Lines        2831     2731     -100     
  Branches      951      916      -35     
==========================================
- Hits         2694     2594     -100     
  Misses        137      137              
Impacted Files Coverage Δ
src/rules/order.js 98.48% <92.00%> (-0.66%) ⬇️
src/rules/newline-after-import.js 95.31% <0.00%> (-0.85%) ⬇️
src/rules/no-cycle.js 97.67% <0.00%> (-0.29%) ⬇️
src/rules/no-unused-modules.js 97.43% <0.00%> (-0.24%) ⬇️
utils/parse.js 98.14% <0.00%> (-0.13%) ⬇️
src/rules/dynamic-import-chunkname.js 96.77% <0.00%> (-0.11%) ⬇️
src/rules/export.js 100.00% <0.00%> (ø)
src/rules/no-restricted-paths.js 100.00% <0.00%> (ø)
src/rules/no-anonymous-default-export.js 100.00% <0.00%> (ø)
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@stropho
Copy link
Contributor

stropho commented May 27, 2022

Hmm wondering what is this waiting for :) Anyway, if I was the maintainer, I would appreciate keeping the test coverage on the same level or higher.

A nice non-blocking improvement could be providing and option to sort type imports before/after the regular import. On the other hand that could be a different PR..

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks, this is great!

We need some more test coverage, and it needs to be rebased (there’s a conflict).

src/rules/order.js Show resolved Hide resolved
src/rules/order.js Outdated Show resolved Hide resolved
src/rules/order.js Outdated Show resolved Hide resolved
tests/src/rules/order.js Outdated Show resolved Hide resolved
tests/src/rules/order.js Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented May 27, 2022

Regarding separate PRs, i don’t think that’s necessary, but it would be nice when rebasing to group the “cleanup” stuff into its own commits, separate from the “fix and updated tests” commits.

@stropho
Copy link
Contributor

stropho commented May 31, 2022

Hey @aaronadamsCA , how are you? What's the status? As this feature is very useful to me right now, I'm willing to help :) If you give me access to your fork, I could move this PR a bit further ;)

@ljharb
Copy link
Member

ljharb commented May 31, 2022

@stropho its only been 4 days over a holiday weekend, give it some time :-)

@aaronadamsCA
Copy link
Contributor Author

Back after 3 weeks off, I'll take a look this week, thanks!

@ljharb ljharb marked this pull request as draft August 29, 2022 19:01
@ljharb
Copy link
Member

ljharb commented Aug 29, 2022

@aaronadamsCA this needs a rebase, and it's not a straightforward one. any chance you'll get to it soon?

@stropho stropho mentioned this pull request Sep 5, 2022
4 tasks
@ljharb ljharb force-pushed the aaronadamsCA/improve-order branch 3 times, most recently from a133e5f to c4f3cc4 Compare September 6, 2022 17:37
@ljharb ljharb merged commit c4f3cc4 into import-js:main Sep 7, 2022
@aaronadamsCA
Copy link
Contributor Author

Sorry, my contributions fell behind for a while due to offline things, so thank you for completing this!

@aaronadamsCA aaronadamsCA deleted the aaronadamsCA/improve-order branch November 8, 2022 17:39
@stropho
Copy link
Contributor

stropho commented Nov 9, 2022

And now, finally publishing it after a while would be great :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

import/order: alphabetize import type above (or below) import
3 participants