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: add consolidateIslands option to collapse excess spacing for aesthetically pleasing imports #3129

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

Xunnamius
Copy link
Contributor

Depends on #3128

This PR implements in import/order: a new option enabling the manipulation/consolidation of newlines around 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

Collapse excess spacing for aesthetically pleasing imports

This is implemented via consolidateIslands. The proposed documentation corresponding to this new feature can be previewed here.

Example

Given this code (which could be the output of a previous --fix pass):

var fs = require('fs');
var path = require('path');
var { util1, util2, util3 } = require('util');

var async = require('async');

// Ugly but technically valid

var relParent1 = require('../foo');
var {
  relParent21,
  relParent22,
  relParent23,
  relParent24,
} = require('../');
var relParent3 = require('../bar');

var { sibling1,
  sibling2, sibling3 } = require('./foo');
var sibling2 = require('./bar');
var sibling3 = require('./foobar');

And the following settings, the rule check will pass:

{
  "newlines-between": "always-and-inside-groups"
}

However, when given the following instead, the rule check will fail:

{
  "newlines-between": "always-and-inside-groups",
+ "consolidateIslands": "inside-groups"
}

With --fix yielding:

var fs = require('fs');
var path = require('path');
var { util1, util2, util3 } = require('util');

var async = require('async');

// Pretty

var relParent1 = require('../foo');

var {
  relParent21,
  relParent22,
  relParent23,
  relParent24,
} = require('../');

var relParent3 = require('../bar');

var { sibling1,
  sibling2, sibling3 } = require('./foo');

var sibling2 = require('./bar');
var sibling3 = require('./foobar');

Note how the intragroup "islands" of grouped single-line imports, as well as multi-line imports, are surrounded by new lines.

Essentially, I was looking for a newlines-between-like setting somewhere between "never" and "always-and-inside-groups". I want newlines separating groups/pathGroups imports from one another (like "always-and-inside-groups"), newlines separating imports that span multiple lines from other imports (this is the new thing), and any remaining newlines deleted or "consolidated" (like "never"). The example above demonstrates this use case.

Right now, this is achievable with newlines-between set to "always-and-inside-groups" if you add additional newlines around multi-line imports to every file by hand. The goal of consolidateIslands is to allow eslint --fix to take care of the tedium in a backward-compatible way.

There was a slight complication with consolidateIslands though: while testing across a few mid-sized repos, I discovered my naive implementation caused a conflict when enabled alongside sortTypesAmongThemselves: true, newlines-between: "always-and-inside-groups", and newlines-between-types: "never"... and then only when a normal import was followed by a multi-line type-only import. This conflict makes sense, since newlines-between-types: "never" wants no newlines ever and demands no newline separate type-only imports from normal imports (since, currently, newlines-between-types governs that space), yet consolidateIslands demands a newline separate all multi-line imports from other imports.

To solve this, the current implementation has newlines-between-types yield to consolidateIslands whenever they conflict. I've also added a test to catch any regressions around this edge case.

Copy link

codecov bot commented Dec 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.68%. Comparing base (5e7ea1d) to head (ba167f2).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3129      +/-   ##
==========================================
+ Coverage   95.51%   95.68%   +0.17%     
==========================================
  Files          83       83              
  Lines        3659     3688      +29     
  Branches     1306     1331      +25     
==========================================
+ Hits         3495     3529      +34     
+ Misses        164      159       -5     

☔ 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 04:41
@Xunnamius Xunnamius force-pushed the contrib-consolidate-islands branch from 02507ca to 464fa71 Compare December 31, 2024 01:18
@Xunnamius Xunnamius force-pushed the contrib-consolidate-islands branch 3 times, most recently from 16bdbf7 to 90cfd85 Compare January 22, 2025 09:33
@Xunnamius Xunnamius marked this pull request as ready for review January 22, 2025 11:27
@ljharb ljharb marked this pull request as draft January 23, 2025 06:45
@Xunnamius Xunnamius force-pushed the contrib-consolidate-islands branch 3 times, most recently from 5afa43a to 3c4c3f2 Compare January 26, 2025 11:00
@ljharb ljharb force-pushed the contrib-consolidate-islands branch from 3c4c3f2 to ba167f2 Compare January 29, 2025 16:44
@ljharb ljharb marked this pull request as ready for review January 29, 2025 16:44
@ljharb ljharb force-pushed the contrib-consolidate-islands branch from ba167f2 to 4f145a2 Compare January 31, 2025 17:17
@ljharb ljharb changed the title [New] order: collapse excess spacing for aesthetically pleasing imports via consolidateIslands [New] order: add consolidateIslands option to collapse excess spacing for aesthetically pleasing imports Jan 31, 2025
@ljharb ljharb merged commit 4f145a2 into import-js:main Jan 31, 2025
341 of 342 checks passed
@Xunnamius Xunnamius deleted the contrib-consolidate-islands branch February 1, 2025 04:48
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.

2 participants