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

[New] order: allow controlling intragroup spacing of type-only imports via newlines-between-types #3127

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Xunnamius
Copy link
Contributor

Depends on #3104

This PR implements in import/order: a new option analogous to newlines-between but specifically for controlling newlines between type-only imports.

A demo package containing this feature is temporarily available for easy testing:

npm install --save-dev eslint-plugin-import@npm:@-xun/eslint-plugin-import-experimental

Allow controlling intragroup spacing of type-only imports

This is implemented via newlines-between-types. The proposed documentation corresponding to this new feature can be previewed here.

Example

Given this code:

import type A from 'fs';
import type B from 'path';
import type C from '../foo.js';
import type D from './bar.js';
import type E from './';

import a from 'fs';
import b from 'path';

import c from '../foo.js';

import d from './bar.js';

import e from './';

And the following settings, the rule check will fail:

{
  "groups": ["type", "builtin", "parent", "sibling", "index"],
  "sortTypesAmongThemselves": true,
  "newlines-between": "always"
}

With --fix yielding:

import type A from 'fs';
import type B from 'path';

import type C from '../foo.js';

import type D from './bar.js';

import type E from './';

import a from 'fs';
import b from 'path';

import c from '../foo.js';

import d from './bar.js';

import e from './';

However, with the following settings, the rule check will succeed instead:

{
  "groups": ["type", "builtin", "parent", "sibling", "index"],
  "sortTypesAmongThemselves": true,
  "newlines-between": "always",
+ "newlines-between-types": "ignore"
}

sortTypesAmongThemselves allows sorting type-only and normal imports separately. By default, newlines-between will govern all newlines between import statements like normal.

I generally want my type-only imports to be sorted for ease of reference but never have newlines between them (save space) while I want my normal imports (which I tend to visually peruse more often) to be aesthetically pleasing, grouped, and sorted. newlines-between is too coarse-grained for this, so this PR introduces newlines-between-types, a setting identical to newlines-between except it only applies to type-only imports, and only when sortTypesAmongThemselves is enabled (i.e. it is backward-compatible).

When newlines-between and newlines-between-types conflict, newlines-between-types takes precedence for type-only imports. For normal imports, newlines-between-types is ignored entirely.

One issue that might warrant further discussion is which setting governs the newline separating type-only imports from normal imports. Right now, I have it so newlines-between-types controls this space, but perhaps it should be its own setting.

Copy link

codecov bot commented Dec 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.12%. Comparing base (341178d) to head (d472b80).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3127      +/-   ##
==========================================
- Coverage   95.59%   92.12%   -3.47%     
==========================================
  Files          83       83              
  Lines        3629     3656      +27     
  Branches     1282     1304      +22     
==========================================
- Hits         3469     3368     -101     
- Misses        160      288     +128     

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

@ljharb ljharb marked this pull request as draft December 23, 2024 17:41
@Xunnamius Xunnamius force-pushed the contrib-newlines-between-types branch from eba9746 to 8256230 Compare December 31, 2024 00:48
@Xunnamius Xunnamius force-pushed the contrib-newlines-between-types branch from 8256230 to 3d64add Compare January 21, 2025 11:19
src/rules/order.js Outdated Show resolved Hide resolved
@Xunnamius
Copy link
Contributor Author

Like with the other PR, codecov is reporting patch coverage at 100% with no uncovered lines.

When I look at "indirect changes" I'm seeing the same src/rules/order.js file (among a bunch of seemingly unrelated files) but with uncovered lines that have nothing to do with the tests added by this PR. This PR also doesn't remove any tests, so I'm not quite sure what's going on there. Could be that some formerly-covered source was deleted. Everything else seems to have passed though :)

@Xunnamius Xunnamius marked this pull request as ready for review January 21, 2025 23:33
@ljharb ljharb marked this pull request as draft January 23, 2025 06:46
@ljharb ljharb marked this pull request as ready for review January 23, 2025 08:08
@ljharb ljharb force-pushed the contrib-newlines-between-types branch 2 times, most recently from 2f2ea6d to 0f3d9a5 Compare January 23, 2025 08:10

> \[!NOTE]
>
> This setting is only meaningful when [`sortTypesGroup`][7] is enabled.
Copy link
Member

Choose a reason for hiding this comment

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

should we use the schema to ensure that this option can't be set to true unless sortTypesGroup is also set to true?

Copy link
Contributor Author

@Xunnamius Xunnamius Jan 23, 2025

Choose a reason for hiding this comment

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

Sounds like a good idea to me. I was not aware that was a thing for eslint plugins :)

Copy link
Member

Choose a reason for hiding this comment

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

it often requires a bit of gymnastics, but it's usually worth it for the DX.

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

Successfully merging this pull request may close these issues.

2 participants