import/order fixer is not working properly?

[AgainPsychoX]

I am trying to use eslint with the plugin in my project, and I was happy when I saw that there are fixers for import ordering, so I don't have to visit hundreds of files to satisfy the linter.

+(fixable) The --fix option on the [command line] automatically fixes problems reported by this rule.

^{(from https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/order.md)}

Unfortunately, in quite a lot of cases it does not work.

I use both plugin:import/recommended and plugin:import/typescript (project uses Typescript/React (.ts/.tsx)).

Rules (only import/* listed):

		"import/no-named-as-default-member": "off",
		"import/newline-after-import": "warn",
		"import/no-duplicates": "warn",
		"import/order": ["warn", {
			"groups": ["builtin", "external", "internal", "parent", "sibling", "index", "object"],
			"pathGroups": [
				{"pattern": "@flyparks/**", "group": "internal", "position": "before"}
			],
			"newlines-between": "never",
			"alphabetize": {
				"order": "desc",
				"caseInsensitive": true
			}
		}]

Settings (only import/* listed):

		"import/resolver": {
			"webpack": {
				"config": "../node_modules/react-scripts/config/webpack.config.js"
			}
		},

Example

File (only imports):

import { useHistory } from 'react-router-dom';
import React, {useEffect, useState } from 'react';
import { BookingEntry, getPathToBookingPage } from '@flyparks/models';
import './AccessBookingNoAuthPage.scss';
import { CircularProgress, TextField } from '@material-ui/core';
import { httpsCallable } from 'firebase/functions';
import FirebaseServices from 'utils/FirebaseServices';

Linter output:

   3:1   warning  There should be no empty line between import groups                           import/order
   7:1   warning  `@material-ui/core` import should occur before import of `@flyparks/models`   import/order
   8:1   warning  `firebase/functions` import should occur before import of `@flyparks/models`  import/order

Using --fix does nothing, despite I imagine removing empty lines should be easy (its obviously detected already, isn't it?).
Reordering the lines also should be quite easy...

Is there issue with Typescript/React files maybe?

[AgainPsychoX]

I thought it was bad resolver... but after changing to webpack it still does not work.

[ljharb]

It’s working fine. Imports without bindings have side effects, and those can’t be safely reordered across by the autofixer.

Put the css import at the top or bottom, and the fixer should be able to handle the rest.

[AgainPsychoX]

I wonder, can I use something like

			"pathGroups": [
				{"pattern": "*.scss", "group": "index", "position": "after"}

to move those on --fix?

Sadly, it does not work :C

[ljharb]

No. Side effecting imports can never, under any circumstances, be safely auto fixed.

[AgainPsychoX]

Maybe it could be feature request? Maybe adding some flag includeSideEffectsImports to pathGroups?

Anyway, for now it looks like I need find all of those and fix them.

# Since I use Windows (with Powershell) I made use of it to find it
ls -R *.tsx,*.ts | sls 'import [''"]'       # to just list occurrences 
# or 
ls -R *.tsx,*.ts | sls 'import [''"]' | % { code $_.Path }      # to open related files in VS Code

But, yeah, moving my .scss imports to top of the imports helped. I opted for top instead bottom because VS Code auto importing feature always places stuff at bottom of the imports list.

[ljharb]

I don't think that's worth it. I think doing it manually is the best and safest approach.

[sk1e]

Experiencing the same issue. I think that safety should not take priority over usability. User could be provided with unsafe option, if that option can really help him.

I have hundreds of components with such css imports and I don't have imports that can change program in any way because of import order changed. Program that depends on import order is a bad program, most likely.

Eslint's fix function could have much higher value in my case and probably many other cases if rule could affect imports with side effects.

[ljharb]

Safety takes priority over everything, always, full stop.

[sk1e]

Well, imports with bindings also could have side effects, so their ordering is not safe either. We can also have imports with bindings and side effects and imports without bindings and without side effects. Existence of bindings does not tell us anything about side effects. Nothing can tell, therefore import ordering can't be safe at all.

We make software to automate some work. It sounds ridiculous to me to do that work manually because it is not safe in cases that are nonsense. It is not safe in other nonsense cases though, so it is even more ridiculous.

[ljharb]

Yes, that is all true - we had to draw a line somewhere, and this is where it's been drawn.

Having side effects in modules that export things is indeed nonsense - if you have this, your application is already irreparably broken. However, a module depending on side effects introduced in a previous import is both not nonsense, and happens all the time in codebases, so it's a perfectly reasonable thing to protect against.

[sk1e]

I still disagree with you and think that there are two good arguments to include imports without bindings in the rule:

1. You say that such dependencies happen all the time in codebases, but for me it never happen. I would say that such dependencies are a sign of a bad, very bad module design. It breaks abstraction in many ways, It even hardly can be named "module" because it becomes not modular. I would never design module like that and I would never use modules like that. So the line you draw affects me in a negative way. I think there are plenty of users like me.
2. It should be up to user to decide if he or she can use unsafe feature, but unsafe feature should be provided anyway if it can help user and there are no safe alternatives. There are a lot examples of software that provide unsafe features: git force push, reset, rebase, dangerouslySetInnerHTML in react elements. If feature considered unsafe we can:
   2.1. Do not include it in default behavior, so none of users will be touched by unsafety unless user explicitly want it.
   2.2. Add "unsafe" prefix to option, so user will take responsibility for using unsafe feature
   2.3 Add mention to documentation that it is unsafe and why it is unsafe.

[AgainPsychoX]

I agree fully with sk1e. Meanwhile:

Safety takes priority over everything, always, full stop.

That's hardly valid an argument to be honest.

Why not leave that possibility for possible feature that some people would find useful? It's not like it's super important thing, I guess it wouldn't be destructive for the plugin design if someone wrote PR for this.

I include .css/.scss from .tsx/.jsx files. While I could make long list of imports in some index.scss linked in the HTML all the time, it would make it hard to automatically drop the .scss in case I stop using the component that used the style. For now, I just moved all .scss declaration on top of the files and it works for me. I won't probably change it to the bottom even if there will be such option, but maybe some people could find it necessary.

---

I guess this issue could be closed, as there is working workaround, at least to mine problem that I described in the first post.

If someone want to have side effects import reordering enabled, one could create new issue as proper feature request - hopefully, after finding this conversation, because sk1e has included few good points in favour of the possible feature.

[AgainPsychoX]

I could add 3. point to the sk1e post above:

3. Such feature could be useful in case if someone want to start using eslint with the import/order plugin while having mess in imports - just like me. I had to go manually through over hundred of files and move the .scss imports to the top. As I didn't have any other side effect imports, I would be useful to have option to move them, especially if there is something like this in the config already (the pathGroups with pattern matching) - just make it work also for unsafe stuff if plugin user wants to.

Of course, again, I don't mean that its important feature, just make it open up to contribution instead drawing hard line before it.

[ljharb]

I do understand that if you've been using nonstandard side-effecting CSS imports in a large codebase, and then you want to suddenly order your imports, that you'll be in a tough spot and need to do lots of manual work.

I'm not sure how to square the safety concerns with the convenience angle.

[sk1e]

@AgainPsychoX I can suggest you prettier plugin for this job. It has some downsides, but works better for me.

[DanielRuf]

I can understand both sides / opinions. But in most cases the order of the css file does not matter as it is not used by the other imports (they are normally not dependent) as these often just contain the styles of the current component.

Not sure why there is such a big resistance to not add an optional config setting (with a warning when used) to move these imports to the top or bottom. Supporting this use case should not be a big problem imho.

And optionally there could be defined which extensions are safe to reorder (css vs js). The latter is generally a problem and should be skipped and printed as warning.

We will also switch to the aforementioned prettier plugin in a big project. Thanks for the link @sk1e.

[ljharb]

@DanielRuf because the order not mattering is uniquely true for css imports - and it would be very brittle to only allow “.css” imports.

[thecodeboss]

The argument I'm seeing is that this option is not being considered because imports without bindings can have side effects, and these side-effects make re-ordering dangerous.

However, as @sk1e pointed out, even imports WITH bindings can have side-effects, and @ljharb you acknowledged this yourself:

Yes, that is all true - we had to draw a line somewhere, and this is where it's been drawn.

It's clear that there are a bunch of people in the community who support having this option and have listed a number of reasons for why it's a valid feature. I don't understand why a line has to be drawn here. I don't think it has to be. Safe defaults are a great idea and I commend your resolve, but allowing flexibility here would better support the needs of the community, and is a win-win for everybody. The defaults can remain safe, while others can manually adjust those settings to what works best in their organization's coding standards.

Thanks @sk1e for suggesting the Prettier plugin, it worked great for me.

[mi-na-bot]

At least something in the docs that having unbound imports will WILDLY BREAK import/order fixes. I just spent HOURS trying to figure out why the plugin was outputting literal garbage.

#2640