Add autofixer for order rule #908

tihonove · 2017-08-02T12:18:04Z

I really want this feature :-)
Fixes #711. Fixes #547.

coveralls · 2017-08-02T12:38:16Z

Coverage increased (+0.05%) to 96.02% when pulling 31def14 on tihonove:master into dd28130 on benmosher:master.

coveralls · 2017-08-02T14:30:32Z

Coverage decreased (-0.006%) to 95.959% when pulling c25bdcb on tihonove:master into dd28130 on benmosher:master.

coveralls · 2017-08-02T14:30:32Z

Coverage decreased (-0.006%) to 95.959% when pulling c25bdcb on tihonove:master into dd28130 on benmosher:master.

coveralls · 2017-08-02T14:30:32Z

Coverage decreased (-0.006%) to 95.959% when pulling c25bdcb on tihonove:master into dd28130 on benmosher:master.

coveralls · 2017-08-02T14:30:32Z

Coverage decreased (-0.006%) to 95.959% when pulling c25bdcb on tihonove:master into dd28130 on benmosher:master.

coveralls · 2017-08-02T15:17:33Z

Coverage increased (+0.2%) to 96.131% when pulling 2209e83 on tihonove:master into dd28130 on benmosher:master.

coveralls · 2017-08-02T15:17:33Z

Coverage increased (+0.2%) to 96.131% when pulling 2209e83 on tihonove:master into dd28130 on benmosher:master.

coveralls · 2017-08-02T15:17:35Z

Coverage increased (+0.2%) to 96.131% when pulling 2209e83 on tihonove:master into dd28130 on benmosher:master.

tihonove · 2017-08-02T15:29:15Z

Hi, i dont have ideas how to fix appveyor build. Could you give me a hint?
Another point: travis builds on MacOS seems to unstable. Here is failed build and after fix coverage success build

ljharb

What happens when there's non-require non-import statements between the mis-ordered items? I wouldn't expect this rule to enforce "all imports/requires are grouped together/at the top"

tihonove · 2017-08-03T06:03:48Z

@ljharb, currently autofixer just fix errors reported by original rule. Code

var async = require('async');
var a = 1;
var fs = require('fs');

transforms to

var fs = require('fs');
var async = require('async');
var a = 1;

due to rule requires 'fs' to occurs before 'async'.
In my opinion fixer should fix errors reported by rule.
Should i modify rule to avoid this behavior?

ljharb · 2017-08-03T06:08:02Z

What about:

var b = require('b');
require('a')(b);

?

In other words, I think this rule is only safely fixable in a subset of the circumstances that the rule warns on - eslint core's policy (which I think everyone should follow) is that autofixes must always be "safe", and not change the meaning of the code, even if that means not autofixing some things.

tihonove · 2017-08-03T06:24:00Z

🙈 Currently autofixer brokes the code above. Im going to update autofixer to avoid unsafe fixes.

ljharb · 2017-08-03T06:32:58Z

Specifically: I think that to start, autofixing can only be said to be safe only within a chunk of requires and/or imports, where the requires are purely assignments - and any code that's not a variable-assigned-require or an import-with-a-binding has to be considered a boundary across which autofixing can't cross.

For example, side-effecting requires/imports (like just import 'foo'; or require('foo');, eg polyfills/shims/etc), are likely in place intentionally, which means that every require/import that happens before it must remain before it, and everything after it must remain after it.

tihonove · 2017-08-14T08:26:14Z

Hi. Currently reorder fixes is very safe and only strict var-assignement-require and imports can be reordered and autofixes would not applied to the following code

var foo = require('./foo').bar.bar.bar;
var fs = require('fs');

Im not sure about this. Maybe such reorderings should be allowed

coveralls · 2017-08-14T08:36:43Z

Coverage increased (+0.2%) to 96.117% when pulling 316d818 on tihonove:master into dd28130 on benmosher:master.

coveralls · 2017-08-14T14:40:32Z

Coverage increased (+0.2%) to 96.118% when pulling 82a66c4 on tihonove:master into dd28130 on benmosher:master.

coveralls · 2017-08-14T14:40:32Z

Coverage increased (+0.2%) to 96.118% when pulling 82a66c4 on tihonove:master into dd28130 on benmosher:master.

ljharb · 2017-08-14T18:29:10Z

@tihonove using fs would make it safe; but if you imported something with side effects it wouldn't be :-/

tihonove · 2017-08-14T18:38:50Z

Ok, are there anything i can do to complete pr?

ljharb

Thanks, this is very thorough.

We'll need to get the tests passing before it can be merged tho.

ljharb · 2017-08-14T18:54:47Z

tests/src/rules/order.js

+    test({
+      code: `
+        var async = require('async');
+        var fs = require('fs');` + ' ' +  `


you could do:

var fs = require('fs');${' '}

without having to use string concatenation

ljharb · 2017-08-14T18:57:49Z

tests/src/rules/order.js

+        message: '`fs` import should occur before import of `async`',
+      }],
+    })),
+    // reorder cant cross variable assignemet


it's not really the variable assignment that's an issue here; it's what's on the RHS - in other words, anything that's depending on a global, or on built-in prototypes, or on any imported value.

Probably safest tho to stick with "variable assignment"

coveralls · 2017-08-16T09:17:56Z

Coverage increased (+0.2%) to 96.145% when pulling 9af8223 on tihonove:master into dd28130 on benmosher:master.

tihonove · 2017-08-16T09:52:00Z

We'll need to get the tests passing before it can be merged tho.

I dont have ideas how to fix appveyor build. Could you give me a hint? Seems it fails after all test successfully passed

ljharb · 2017-08-16T21:11:04Z

I believe the appveyor build might be broken on master; @benmosher, can you confirm?

lukeapage · 2017-09-03T09:16:15Z

Would be great to see this merged..

lukeapage · 2017-09-16T07:33:48Z

@benmosher any chance of merging and releasing this?

lukeapage · 2017-10-23T18:47:20Z

@tihonove - master build is now passing again
@benmosher - any chance you can take a look at merging this?

coveralls · 2017-10-26T14:12:46Z

Coverage increased (+0.1%) to 96.364% when pulling 41c4540 on tihonove:master into 1eac942 on benmosher:master.

mrm007 · 2017-12-08T05:26:27Z

It sure would be great to have this merged 🙏

lukeapage · 2018-01-15T05:03:15Z

@ljharb I see you have merge rights now. Given you approved this and Ben does not seem to be about, would you consider merging it? Thanks.

ljharb · 2018-01-15T06:53:31Z

@lukeapage i've had them this whole time; I don't think an autofixer - especially one as risky as an ordering autofixer - should be merged with only one collaborator having reviewed it.

MoOx · 2018-03-27T12:16:36Z

@benmosher anything we can do to make this review easy? It could be really nice to get this in a release :)

MoOx

I just tried this on a pretty big codebase, worked like a charm without any issues!

MoOx · 2018-03-27T12:17:17Z

CHANGELOG.md

@@ -9,6 +9,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
 ## [2.8.0] - 2017-10-18
 ### Added
 - [`exports-last`] rule ([#620] + [#632], thanks [@k15a])
+- Autofixer for [`order`] rule ([#711], thanks [@tihonove])


Might need to be adjusted

benmosher

not super familiar with fixers or this rule in particular, but tests look good and @MoOx's anecdote about running it on a big project is a big deal. thanks for this!

benmosher · 2018-03-28T00:45:32Z

will merge once ~~linux~~ builds pass 👍(I finally fixed the windows build)

MoOx · 2018-03-28T13:04:42Z

macOS build seems to be struggling. Are those really necessary btw (as this lib doesn't involve any "macos" specific code?)

benmosher · 2018-03-28T15:54:29Z

@MoOx I agree that it shouldn't matter but the tests are there for the times when it mysteriously does. 😅 LGTM now, though 👍

ljharb · 2018-04-19T13:36:17Z

See #1086 - this PR caused a performance regression. It’d be great if anyone interested in keeping this autofixer in the plugin could look into that issue.

ajhyndman · 2018-05-16T04:38:28Z

You're my hero, @tihonove! I just needed to apply an import/order rule to 395 files; this saved me a full day of hair pulling.

tihonove mentioned this pull request Aug 2, 2017

A lot of improvements AlexJuarez/eslint-plugin-import-order-autofix#2

Closed

ljharb reviewed Aug 2, 2017

View reviewed changes

ljharb reviewed Aug 14, 2017

View reviewed changes

ljharb approved these changes Aug 16, 2017

View reviewed changes

ljharb requested review from benmosher and jfmengels August 16, 2017 21:11

ljharb added the import/export ordering label Oct 23, 2017

add fixer for order rule

09703d0

tihonove force-pushed the master branch from f3e31a7 to 09703d0 Compare October 26, 2017 13:05

ljharb mentioned this pull request Mar 25, 2018

order rule should be fixable #1051

Closed

MoOx approved these changes Mar 27, 2018

View reviewed changes

benmosher added 3 commits March 27, 2018 20:38

pre-merge changelog twiddling

a6827fe

missed the link to import-js#711

066d2d3

Merge branch 'master' into master

41c4540

benmosher reviewed Mar 28, 2018

View reviewed changes

benmosher merged commit b34d9ff into import-js:master Mar 28, 2018

renovate bot mentioned this pull request Mar 30, 2018

Update eslint-plugin-import to 2.12.0 sbardian/DebtTrackerAPI#25

Merged

robertrossmann mentioned this pull request Apr 7, 2018

Add eslint-plugin-import-order-autofix strvcom/eslint-config-javascript#30

Closed

renovate bot mentioned this pull request Apr 17, 2018

Update dependency eslint-plugin-import to v2.11.0 Urigo/tortilla#68

Merged

ljharb mentioned this pull request Apr 18, 2018

Sort-imports autofixer #547

Closed

JakubJagoda mentioned this pull request Apr 19, 2018

Memory issues with import/order autofixer in 2.11.0 #1086

Closed

magicmark mentioned this pull request Apr 22, 2018

Enable new eslint rules sharkcore/eslint-config#13

Merged

nevir mentioned this pull request Apr 30, 2018

Fix #389: Allow secondary alphabetical sorting #629

Merged

amannn mentioned this pull request Nov 15, 2018

Remove eslint-plugin-import-order-autofix molindo/eslint-config-molindo#10

Closed

ljharb mentioned this pull request Jan 3, 2019

autofix "import/order" will produce new problems? #1252

Closed

Add autofixer for order rule #908

Add autofixer for order rule #908

Conversation

tihonove commented Aug 2, 2017 • edited by ljharb

coveralls commented Aug 2, 2017 • edited

coveralls commented Aug 2, 2017 • edited

coveralls commented Aug 2, 2017

coveralls commented Aug 2, 2017

coveralls commented Aug 2, 2017

coveralls commented Aug 2, 2017

coveralls commented Aug 2, 2017

coveralls commented Aug 2, 2017 • edited

tihonove commented Aug 2, 2017 • edited

ljharb left a comment

Choose a reason for hiding this comment

tihonove commented Aug 3, 2017

ljharb commented Aug 3, 2017

tihonove commented Aug 3, 2017

ljharb commented Aug 3, 2017

tihonove commented Aug 14, 2017

coveralls commented Aug 14, 2017 • edited

coveralls commented Aug 14, 2017

coveralls commented Aug 14, 2017 • edited

ljharb commented Aug 14, 2017

tihonove commented Aug 14, 2017

ljharb left a comment

Choose a reason for hiding this comment

ljharb Aug 14, 2017

Choose a reason for hiding this comment

ljharb Aug 14, 2017

Choose a reason for hiding this comment

coveralls commented Aug 16, 2017 • edited

tihonove commented Aug 16, 2017

ljharb commented Aug 16, 2017

lukeapage commented Sep 3, 2017

lukeapage commented Sep 16, 2017

lukeapage commented Oct 23, 2017

coveralls commented Oct 26, 2017 • edited

mrm007 commented Dec 8, 2017

lukeapage commented Jan 15, 2018

ljharb commented Jan 15, 2018

MoOx commented Mar 27, 2018

MoOx left a comment

Choose a reason for hiding this comment

MoOx Mar 27, 2018

Choose a reason for hiding this comment

benmosher left a comment

Choose a reason for hiding this comment

benmosher commented Mar 28, 2018 • edited

MoOx commented Mar 28, 2018

benmosher commented Mar 28, 2018

ljharb commented Apr 19, 2018

ajhyndman commented May 16, 2018

tihonove commented Aug 2, 2017 •

edited by ljharb

coveralls commented Aug 2, 2017 •

edited

coveralls commented Aug 2, 2017 •

edited

coveralls commented Aug 2, 2017 •

edited

tihonove commented Aug 2, 2017 •

edited

coveralls commented Aug 14, 2017 •

edited

coveralls commented Aug 14, 2017 •

edited

coveralls commented Aug 16, 2017 •

edited

coveralls commented Oct 26, 2017 •

edited

benmosher commented Mar 28, 2018 •

edited