`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.