Skip to content

Commit

Permalink
Revert 1fa2971 (breaking group change in order)
Browse files Browse the repository at this point in the history
1fa2971 changed the way groups work when there is only one, leading
to single nested groups being treated as though they were unnested.
Prior to this, if you wanted to group e.g. builtin and external
imports together at the top and everything else together as a second
group, a single nested group was the way to do it

[It appears that this change was unintentional][1], and was made to
try to fix what seems to be a misunderstanding around nested groups
([#2687]). [The docs][2] continue to suggest that nested groups
should be "mingled together" and makes no reference to a single nested
group with no other groups being an invalid option

This therefore reverts the change to how groups work when there is
only one. No documentation change should be necessary given this is
already the described behaviour

[1]: #2687 (comment)
[2]: https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/order.md#groups-array
  • Loading branch information
yndajas authored and ljharb committed Aug 9, 2023
1 parent e9de30a commit 0847443
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 39 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange

## [Unreleased]

### Fixed
- [`order`]: revert breaking change to single nested group ([#2854], thanks [@yndajas])

### Changed
- [Docs] remove duplicate fixable notices in docs ([#2850], thanks [@bmish])

Expand Down Expand Up @@ -1082,6 +1085,7 @@ for info on changes for earlier releases.

[`memo-parser`]: ./memo-parser/README.md

[#2854]: https://github.com/import-js/eslint-plugin-import/pull/2854
[#2850]: https://github.com/import-js/eslint-plugin-import/pull/2850
[#2842]: https://github.com/import-js/eslint-plugin-import/pull/2842
[#2835]: https://github.com/import-js/eslint-plugin-import/pull/2835
Expand Down Expand Up @@ -1887,5 +1891,6 @@ for info on changes for earlier releases.
[@wtgtybhertgeghgtwtg]: https://github.com/wtgtybhertgeghgtwtg
[@xM8WVqaG]: https://github.com/xM8WVqaG
[@xpl]: https://github.com/xpl
[@yndajas]: https://github.com/yndajas
[@yordis]: https://github.com/yordis
[@zloirock]: https://github.com/zloirock
4 changes: 0 additions & 4 deletions src/rules/order.js
Original file line number Diff line number Diff line change
Expand Up @@ -422,10 +422,6 @@ const types = ['builtin', 'external', 'internal', 'unknown', 'parent', 'sibling'
// Example: { index: 0, sibling: 1, parent: 1, external: 1, builtin: 2, internal: 2 }
// Will throw an error if it contains a type that does not exist, or has a duplicate
function convertGroupsToRanks(groups) {
if (groups.length === 1) {
// TODO: remove this `if` and fix the bug
return convertGroupsToRanks(groups[0]);
}
const rankObject = groups.reduce(function (res, group, index) {
[].concat(group).forEach(function (groupItem) {
if (types.indexOf(groupItem) === -1) {
Expand Down
61 changes: 26 additions & 35 deletions tests/src/rules/order.js
Original file line number Diff line number Diff line change
Expand Up @@ -2753,7 +2753,7 @@ context('TypeScript', function () {
};

ruleTester.run('order', rule, {
valid: [
valid: [].concat(
// #1667: typescript type import support

// Option alphabetize: {order: 'asc'}
Expand Down Expand Up @@ -2962,7 +2962,31 @@ context('TypeScript', function () {
},
],
}),
],
isCoreModule('node:child_process') && isCoreModule('node:fs/promises') ? [
test({
code: `
import express from 'express';
import log4js from 'log4js';
import chpro from 'node:child_process';
// import fsp from 'node:fs/promises';
`,
options: [{
groups: [
[
'builtin',
'external',
'internal',
'parent',
'sibling',
'index',
'object',
'type',
],
],
}],
}),
] : [],
),
invalid: [].concat(
// Option alphabetize: {order: 'asc'}
test({
Expand Down Expand Up @@ -3218,39 +3242,6 @@ context('TypeScript', function () {
// { message: '`node:fs/promises` import should occur before import of `express`' },
],
}),

test({
code: `
import express from 'express';
import log4js from 'log4js';
import chpro from 'node:child_process';
// import fsp from 'node:fs/promises';
`,
output: `
import chpro from 'node:child_process';
import express from 'express';
import log4js from 'log4js';
// import fsp from 'node:fs/promises';
`,
options: [{
groups: [
[
'builtin',
'external',
'internal',
'parent',
'sibling',
'index',
'object',
'type',
],
],
}],
errors: [
{ message: '`node:child_process` import should occur before import of `express`' },
// { message: '`node:fs/promises` import should occur before import of `express`' },
],
}),
] : [],
),
});
Expand Down

0 comments on commit 0847443

Please sign in to comment.