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.

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

[Fix] import/order: Support side-effect modules #2961

Closed
wants to merge 1 commit into from

Conversation

M4rm41d3
Copy link

@ljharb
Copy link
Member

ljharb commented Feb 10, 2024

Of course they're valid - but they're only used for side effects, and it's simply never safe for a linter to automatically reorder things across a side-effecting import. Thus, you are intentionally and forever required to manually adjust it.

@ljharb ljharb closed this Feb 10, 2024
@M4rm41d3
Copy link
Author

Of course they're valid - but they're only used for side effects, and it's simply never safe for a linter to automatically reorder things across a side-effecting import. Thus, you are intentionally and forever required to manually adjust it.

Good point. It's probably not intended that the rule then enforces order on these imports then... The rule is enforcing them to be in order erroneously, as you have pointed out.

@ljharb
Copy link
Member

ljharb commented Feb 11, 2024

It is intended - if the order matters, you need to use an eslint override comment.

@M4rm41d3
Copy link
Author

It is intended - if the order matters, you need to use an eslint override comment.

Then why not allow an auto fix and ignore if aslant override commented?

If the rule is telling me it is wrong, I would expect it to be able to auto fix for the same reason. The plugin is saying it should go somewhere else, allow it to move it there.

I'm not sure I understand the philosophy here.

@ljharb
Copy link
Member

ljharb commented Feb 11, 2024

That expectation is simply incorrect, I'm not sure what to tell you. There are many linting warnings that require manual correction, or to be overridden - autofixing is reserved only for the safest possible changes.

@M4rm41d3
Copy link
Author

That expectation is simply incorrect, I'm not sure what to tell you. There are many linting warnings that require manual correction, or to be overridden - autofixing is reserved only for the safest possible changes.

There exists only one single line where that import can go per the rule – so why would it be incorrect, per the logic of the rule, to have the autofixer move the import to that line?

I would agree with you if there existed ambiguity; but per the constraints of the rule there is none.

I am not aware of any lint rules that intentionally do not autofix despite there being no ambiguity in the resolution.

@ljharb
Copy link
Member

ljharb commented Feb 11, 2024

There is ambiguity.

Imagine you have import a from 'a'; import 'c'; import b from 'b';. It is possible that c has a side effect, that b requires in order to work on evaluation.

If eslint autofixes c to go below b, your program will be broken.

It's simply impossible for eslint to know this one way or the other - it's ambiguous - and thus, you must resolve it manually.

@M4rm41d3
Copy link
Author

You could use that example to say this entire rule should have no autofix for any imports, as named exports can also have side effects, see:
https://stackblitz.com/edit/es6-js-p9yvbw?file=import-a.js,import-b.js,index.js

@M4rm41d3
Copy link
Author

M4rm41d3 commented Feb 11, 2024

It's simply impossible for eslint to know this one way or the other - it's ambiguous - and thus, you must resolve it manually.

Yes, then the linter should not be telling me that the import is in the incorrect order. I should not need to ignore it with an eslint disable comment. The rule is telling me there is no ambiguity, it is providing certain instruction on where the import should go.

@ljharb
Copy link
Member

ljharb commented Feb 11, 2024

The rule is configured such that it is in the incorrect order. The two things are in conflict - your code, and your config. You have to fix one of them - and if you want to fix neither, you have to use an override comment.

This is just how eslint rules work. There are many in this category, where they can sometimes autofix, but sometimes you have to fix it manually.

@M4rm41d3
Copy link
Author

M4rm41d3 commented Feb 11, 2024

Why are named exports excluded from this? Any import statement can have side effects, as shown in the above example.

@ljharb
Copy link
Member

ljharb commented Feb 11, 2024

They aren't - import { a } from 'a' would be autofixed as well.

If you're asking about why any imports are safe to move, technically none of them are. However, it's pretty universal that imports with side effects have no exports, and imports with exports have no side effects, so in practice this has not been a problem for nearly a decade.

@levithomason
Copy link

levithomason commented Feb 24, 2024

I also need this. I want to sort import "./foo.css" at the top. It is not intuitive that these are excluded. Especially when adding a pathGroup pattern explicitly matching them.

Please let the repo author decide what pattern best fits their case. In my case, sorting these imports is completely safe and useful.

Nothing stops someone from putting side-effects in a module with default/named exports either. So, not sorting these modules doesn't truly stop sorting of side-effects, it just doesn't sort some imports.

Please reconsider and perhaps at least offer an option to include these, such as a side-effects group.

@ljharb
Copy link
Member

ljharb commented Feb 24, 2024

@levithomason you absolutely can do that. it's just that this plugin won't autofix it, ever.

@M4rm41d3
Copy link
Author

If the rule is confident the autofix is not valid, why is the rule confident the order is incorrect? Can we not exclude these, or provide granular configuration to support these use cases?

Adding an eslint ignore comment shouldn’t be a requirement by design.

@ljharb
Copy link
Member

ljharb commented Feb 25, 2024

@M4rm41d3 it’s not; the typical case is just that you reorder it manually.

Most good linting rules aren’t autofixable - the point is to stop you so you can learn to do the right thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants