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.

Sign up for GitHub

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

Jump to bottom

[import/order] Fix incorrect import reordering #1253

Merged
merged 1 commit into from Oct 9, 2019
Merged

[import/order] Fix incorrect import reordering #1253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -8,14 +8,13 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
### Added
- [`group-exports`]: make aggregate module exports valid ([#1472], thanks [@atikenny])
- [`no-namespace`]: Make rule fixable ([#1401], thanks [@TrevorBurnham])

### Added
- support `parseForESLint` from custom parser ([#1435], thanks [@JounQin])
- [`no-extraneous-dependencies`]: Implement support for [bundledDependencies](https://npm.github.io/using-pkgs-docs/package-json/types/bundleddependencies.html) ([#1436], thanks [@schmidsi]))

### Fixed
- `default`: make error message less confusing ([#1470], thanks [@golopot])
- Support export of a merged typescript namespace declaration ([#1495], thanks [@benmunro])
- [`import/order`]: fix autofix to not move imports across fn calls ([#1253], thanks [@tihonove])

## [2.18.2] - 2019-07-19
- Skip warning on type interfaces ([#1425], thanks [@lencioni])
Expand Down Expand Up @@ -651,6 +650,7 @@ for info on changes for earlier releases.
[#1290]: https://github.com/benmosher/eslint-plugin-import/pull/1290
[#1277]: https://github.com/benmosher/eslint-plugin-import/pull/1277
[#1257]: https://github.com/benmosher/eslint-plugin-import/pull/1257
[#1253]: https://github.com/benmosher/eslint-plugin-import/pull/1253
[#1235]: https://github.com/benmosher/eslint-plugin-import/pull/1235
[#1234]: https://github.com/benmosher/eslint-plugin-import/pull/1234
[#1232]: https://github.com/benmosher/eslint-plugin-import/pull/1232
Expand Down Expand Up @@ -995,3 +995,4 @@ for info on changes for earlier releases.
[@schmidsi]: https://github.com/schmidsi
[@TrevorBurnham]: https://github.com/TrevorBurnham
[@benmunro]: https://github.com/benmunro
[@tihonove]: https://github.com/tihonove
6 changes: 4 additions & 2 deletions src/rules/order.js
View file
Open in desktop
Expand Up @@ -162,8 +162,10 @@ function canCrossNodeWhileReorder(node) {

function canReorderItems(firstNode, secondNode) {
const parent = firstNode.parent
const firstIndex = parent.body.indexOf(firstNode)
const secondIndex = parent.body.indexOf(secondNode)
const [firstIndex, secondIndex] = [
parent.body.indexOf(firstNode),
parent.body.indexOf(secondNode),
].sort()
const nodesBetween = parent.body.slice(firstIndex, secondIndex + 1)
for (var nodeBetween of nodesBetween) {
if (!canCrossNodeWhileReorder(nodeBetween)) {
Expand Down
157 changes: 157 additions & 0 deletions tests/src/rules/order.js
View file
Open in desktop
Expand Up @@ -1265,7 +1265,137 @@ ruleTester.run('order', rule, {
},
],
}),
// reorder fix cannot cross function call on moving below #1
test({
code: `
const local = require('./local');

fn_call();

const global1 = require('global1');
const global2 = require('global2');

fn_call();
`,
output: `
const local = require('./local');

fn_call();

const global1 = require('global1');
const global2 = require('global2');
Copy link
Member

Choose a reason for hiding this comment

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

without the fix, i get this output (and a test failure):

AssertionError [ERR_ASSERTION] [ERR_ASSERTION]: Output is incorrect.
+ expected
- actual
 
+        const local = require('./local');
 
         fn_call();
 
         const global1 = require('global1');
         const global2 = require('global2');
-        const local = require('./local');
 
         fn_call();


fn_call();
`,
errors: [{
ruleId: 'order',
message: '`./local` import should occur after import of `global2`',
}],
}),
// reorder fix cannot cross function call on moving below #2
test({
code: `
const local = require('./local');
Copy link
Member

Choose a reason for hiding this comment

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

AssertionError [ERR_ASSERTION] [ERR_ASSERTION]: Output is incorrect.
+ expected
- actual
 
+        const local = require('./local');
         fn_call();
         const global1 = require('global1');
         const global2 = require('global2');
-        const local = require('./local');
 
         fn_call();

fn_call();
const global1 = require('global1');
const global2 = require('global2');

fn_call();
`,
output: `
const local = require('./local');
fn_call();
const global1 = require('global1');
const global2 = require('global2');

fn_call();
`,
errors: [{
ruleId: 'order',
message: '`./local` import should occur after import of `global2`',
}],
}),
// reorder fix cannot cross function call on moving below #3
test({
code: `
const local1 = require('./local1');
Copy link
Member

Choose a reason for hiding this comment

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

AssertionError [ERR_ASSERTION] [ERR_ASSERTION]: Output is incorrect.
+ expected
- actual
 
+        const local1 = require('./local1');
         const local2 = require('./local2');
         const local3 = require('./local3');
         const local4 = require('./local4');
         fn_call();
         const global2 = require('global2');
         const global3 = require('global3');
         const global4 = require('global4');
         const global5 = require('global5');
-        const local1 = require('./local1');
         fn_call();

const local2 = require('./local2');
const local3 = require('./local3');
ljharb marked this conversation as resolved.
Show resolved Hide resolved
const local4 = require('./local4');
fn_call();
const global1 = require('global1');
const global2 = require('global2');
const global3 = require('global3');
const global4 = require('global4');
const global5 = require('global5');
fn_call();
`,
output: `
const local1 = require('./local1');
const local2 = require('./local2');
const local3 = require('./local3');
const local4 = require('./local4');
fn_call();
const global1 = require('global1');
const global2 = require('global2');
const global3 = require('global3');
const global4 = require('global4');
const global5 = require('global5');
fn_call();
`,
errors: [
'`./local1` import should occur after import of `global5`',
'`./local2` import should occur after import of `global5`',
'`./local3` import should occur after import of `global5`',
'`./local4` import should occur after import of `global5`',
],
}),
// reorder fix cannot cross function call on moving below
test(withoutAutofixOutput({
code: `
const local = require('./local');
Copy link
Member

Choose a reason for hiding this comment

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

AssertionError [ERR_ASSERTION] [ERR_ASSERTION]: Output is incorrect.
+ expected
- actual
 
+        const local = require('./local');
         const global1 = require('global1');
         const global2 = require('global2');
         fn_call();
         const global3 = require('global3');
-        const local = require('./local');
 
         fn_call();

const global1 = require('global1');
const global2 = require('global2');
fn_call();
const global3 = require('global3');

fn_call();
`,
errors: [{
ruleId: 'order',
message: '`./local` import should occur after import of `global3`',
}],
})),
// reorder fix cannot cross function call on moving below
// fix imports that not crosses function call only
test({
code: `
const local1 = require('./local1');
const global1 = require('global1');
const global2 = require('global2');
fn_call();
const local2 = require('./local2');
Copy link
Member

Choose a reason for hiding this comment

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

AssertionError [ERR_ASSERTION] [ERR_ASSERTION]: Output is incorrect.
+ expected
- actual
 
+        const local1 = require('./local1');
         const global1 = require('global1');
         const global2 = require('global2');
         fn_call();
-        const local2 = require('./local2');
         const global3 = require('global3');
         const global4 = require('global4');
-        const local1 = require('./local1');
+        const local2 = require('./local2');
 
         fn_call();

const global3 = require('global3');
const global4 = require('global4');

fn_call();
`,
output: `
const local1 = require('./local1');
const global1 = require('global1');
const global2 = require('global2');
fn_call();
const global3 = require('global3');
const global4 = require('global4');
const local2 = require('./local2');

fn_call();
`,
errors: [
'`./local1` import should occur after import of `global4`',
'`./local2` import should occur after import of `global4`',
],
}),
// reorder fix cannot cross non import or require
test(withoutAutofixOutput({
code: `
Expand All @@ -1278,6 +1408,33 @@ ruleTester.run('order', rule, {
message: '`fs` import should occur before import of `async`',
}],
})),
// reorder fix cannot cross function call on moving below (from #1252)
test({
code: `
const env = require('./config');

Object.keys(env);
Copy link
Member

Choose a reason for hiding this comment

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

AssertionError [ERR_ASSERTION] [ERR_ASSERTION]: Output is incorrect.
+ expected - actual
 
+        const env = require('./config');
 
         Object.keys(env);
 
         const http = require('http');
         const express = require('express');
-        const env = require('./config');
 
         http.createServer(express());


const http = require('http');
const express = require('express');

http.createServer(express());
`,
output: `
const env = require('./config');

Object.keys(env);

const http = require('http');
const express = require('express');

http.createServer(express());
`,
errors: [{
ruleId: 'order',
message: '`./config` import should occur after import of `express`',
}],
}),
// reorder cannot cross non plain requires
test(withoutAutofixOutput({
code: `
Expand Down