Do not reorder side effect nodes #110

[blutorange]

This PR resolves #110

• The relative order of side effect and non side effect imports is now preseverd by default. See the readme and issue for more info.
• I renamed the getSortedNodes method to getSortedNodesByImportOrder; and added a new implementation for getSortedNodes that first splits the imports at each side effect node, then sorts each chunk via the original getSortedNodesByImportOrder. I also moved the logic for adjusting comments and inserting new lines to the new getSortedNodes method.
• With the exception of one test case (imports-with-comments-and-third-party has a side effect import import "./commands"), all existing tests still pass without any modifications. This leads me to believe you haven't considered these kind of import statements yet and how they should be treated. So this PR could be thought of as defining the behavior for that scenario.
• I also added a few additional tests for the new behavior with side effect imports.

The way this PR is implemented (only performing less sorting in case of side effect imports) means that all source files that were considered formatted correctly before this PR are still considered to be formatted correctly. So this change should not require a new major version (assuming you are following semver). Also, prettier recommends users to use an exact version, so the same should also apply to plugins.

That only leaves the question of whether (a) we should introduce an option that can turn side effect import sorting on and off, and (b) if so, what should be the default. My opinion is no, following the philosophy of prettier, which is to be an opinionated code formatter without many options. Not sorting side effects should also be the default, as having side effects is the only purpose of those imports and changing their order is more likely to break the code.

[IanVS]

@ayusharma @byara what do you think of this idea? I'd really love to have this merged and released, as without it I'm not able to use this excellent-looking plugin. Thanks!

For whatever it's worth, I built the files from this branch and plopped them into my node_modules, and they did exactly what I wanted, with no other ill effects.

[byara]

Hey @blutorange @IanVS thanks for the PR. This seems like a really good idea. But I have to say that we have to take a look into it and at the moment both @ayusharma and I are on holiday 😅 As soon as we get back, we will take care of this 👍

[blutorange]

No problem, take your time and enjoy your holidays, I can wait a little while. Let me know what you think when get back : )

[IanVS]

Yeah I'm all set for now, using patch-package to apply it to my node_modules. I'll report back if I see anything funky, but in the meantime, enjoy your time off!

[artola]

@byara @ayusharma do you have new about this PR ? ... it is okayish ? because it solves 1 of the 3 issues that prevents us from using the plugin.
if this one moves on, I will give it a try to the others too.

[blutorange]

Yeah, if there's anything I can do to improve this PR, let me know. I have some index files too that need side effect imports where getting this merged would help greatly.

[byara]

Thank you so much for implementing this feature. I left a few comments / questions regarding some of the changes.
Additionally, I have another request. Inside snapshot testing, please do not modify the current tests to add the side effect tests (ImportsNotSeparated, ImportsSeparatedRest, ImportsSeparated). Simply add a new folder for snapshot testing this feature and and add your tests there with all the combinations possible.

[blutorange]

@byara Should be good now, but let me now if you find anything else.

[byara]

Hey @ayusharma, would be great if you could leave a review as well so we can go forward with this PR.
Maybe we should merge this to v3.2.x branch?

[ayusharma]

Very minor change, nice and clean code. Thank you so much for it ❤️

[ayusharma]

I'll recommend merging it to the 3.2.x branch and release from that branch.

[ayusharma]

Hi, We have to revert the PR as it fails the basic idea of the plugin.

For example:
Input

import React, {
    FC,
    useEffect,
    useRef,
    ChangeEvent,
    KeyboardEvent,
} from 'react';
import { logger } from '@core/logger';
import { reduce, debounce } from 'lodash';
import { Message } from '../Message';
import { createServer } from '@server/node';
import { Alert } from '@ui/Alert';
import 'lodash';
import { repeat, filter, add } from './utils';
import { initializeApp } from '@core/app';
import { Popup } from '@ui/Popup';
import 'styles.css';
import something from 'rx';
import { createConnection } from '@server/database';

With your PR we produce the following output:

import { debounce, reduce } from 'lodash';
import React, {
    ChangeEvent,
    FC,
    KeyboardEvent,
    useEffect,
    useRef,
} from 'react';
import { createServer } from '@server/node';
import { logger } from '@core/logger';
import { Alert } from '@ui/Alert';
import { Message } from '../Message'; // this should be just after import { add, filter, repeat } from './utils';
import 'lodash'; // this should be on the top with the 3rd party imports. 
import { initializeApp } from '@core/app';
import { Popup } from '@ui/Popup';
import { add, filter, repeat } from './utils';
import 'styles.css';
import something from 'rx'; // this should be on the top with the 3rd party imports. 
import { createConnection } from '@server/database'; 

The new sorting breaks the general principle of the 3rd party modules and local modules.

[artola]

@ayusharma I would say that you are wrong in the comments above. Side-effects import 'lodash'; or import 'styles.css'; create new boundaries, you do not know what is ongoing inside, therefore nothing should move from above to below them.
I do believe that the PR is correct, it is not easy to understand, but if the developer write a side-effect it somehow states a condition that must be respected by the plugin.
Of course, it is a contrived "input" nobody will write import 'styles.css'; in the middle of the imports, it is common to do it just on top or bottom of them, but it is a good test to show that the contract is honored.

Look this case, before this PR "polyfills" was moved to the bottom, and therefore the code in "foo" might break. With this PR I expect that side-effects create "boundaries" and the plugin continue to work but within that boundaries, then these imports stay exactly as written.

import './polyfills';
import foo from '../../../foo';

[blutorange]

Hmm, the PR is correct in the sense that it implements the solution described in #110

Whether or not that solution is appropriate is something I'm open to discuss. There are three points I had in mind when I suggested the solution:

• Err on the side of correctness. Having pretty formatted code is great, but only if it does not break it (which I think agrees with prettier's philosophy)
• Keep the number of options to a minimum (also following prettier's philosophy here). We could add several options for people to configure how side effect imports are treated, but in my opinion that adds complexity, and I think it should just work out of the box. And at the very least, it should not break code out of the box.
• An import like import "..." does not import anything, the only reason for it to exist is for side effects. We do not know what those side effects will be -- they could change or prepare the runtime environment in a way that will effect other imports. So to be safe, other imports before a side effect import must stay before it, and other imports after a side effect import must stay after it. The example from artola above with the polyfill ilustrates this.

If this doesn't get accepted, we need to think about another solution. Right now I (and other people apparently) can't use this plugin because it breaks our code. And at least with the default options that should not happen.

---

The ESLint plugin rule says the following

Warns when unassigned imports are out of order. These warning will not be fixed with --fix because unassigned imports are used for side-effects and changing the import of order of modules with side effects can not be done automatically in a way that is safe.

---

[ayusharma]

Hi @artola and @blutorange, Thanks for the explanation. The plugin is out for a quite while and the new change of side effects imports can be distracting and breaking for our users. After a discussion with @byara, We would like to have your changes with a configurable API and we propose two solutions that can be implemented in this PR.

1. File-based API detection: It can be a special comment in the file which tells the plugin to use side effects imports sorting. This sorting will be file-based and will not change the other files in a project. The comment could be // @trivago/prettier-plugin-sort-import side-effects-import-sorting enable or /* @trivago/prettier-plugin-sort-import side-effects-import-sorting enable */. Please make sure that the comment is a top-level comment in the file.

2. Side effects import soring using prettier config: You can introduce a new API in prettier config to enable or disable the side effects import nodes sorting. I would like to keep it disabled by default. You may check out the APIs here

   prettier-plugin-sort-imports/src/index.ts

   Line 15 in e83f3f1

   importOrderCaseInsensitive: {

It would make sense to implement both solutions together. Also, I would like to say thank you again for your work and patience. Please let us know if you have any more questions. 🙏

[JordanSekky]

Hi @ayusharma, I don't agree that introducing this change would be distracting or breaking for your users. Currently, the plugin handles side-effect imports improperly, reordering them and potentially breaking any code this plugin is applied to. This makes the plugin completely unusable for any folks (like me) who have side effect imports.

From what I can tell, this change just makes the plugin usable for any files with side effect imports, with the downside of not totally sorting the imports in files with side-effect imports.

I don't understand what the use-case would be for a config value to disable this feature. Who would disable it?

[artola]

@ayusharma This is not a breaking change, it is a just bug fix. Former behaviour is just improper. Furthermore, it does not break previous saved code because it will keep any side-effect in place.
At most it makes sense to release this change as v4 to "shout" the change.

[blutorange]

I'd consider it a bug too (this is somewhat debatable since every import could have side effects and by that logic, the entire plugin would be a bug -- but import "..." must have side effects to be meaningful) and I'd say the question is how to fix it in such a way that users updating the plugin won't be affected negatively.

But I can understand your point of view as well, let me share my thoughts on you suggestions:

imports can be distracting and breaking for our users

Not sure about distracting, but regarding the breaking: It should not be breaking change in the sense that every file that was recognized by prettier as properly formatted will still be recognized as properly formatted. So users won't get any build failures from updating the plugin. It's just that files that weren't formatted properly before are now not sorted as much.

File-based API detection

Personally I don't like non-standard magic comments that are used only by a one of the many dependencies of a project. It also ties my source files too closely to a particular implementation, making it harder to switch to another implementation / plugin for sorting imports.

I'm not totally opposed to it, but every feature adds complexity and I'm not sure if configuring it per file is really needed. Especially since the main reason for not making this the default behavior is for people who are used to the old behavior.

Side effects import sorting using prettier config

That I think sounds much better. I'd still like if it were be enabled by default so the plugin would not break code out of the box with the default options -- but perhaps we could make the option disabled by default for versions 3.x and make it enabled by default with 4.x ?

[IanVS]

I'll just chime in that I tried out this sorting plugin, thought everything was great, and then later realized that my app was broken because side effect imports were re-arranged. Luckily I did notice before I pushed to production, but it was definitely unfortunate. I wouldn't be able to use this plugin at all, if it weren't for this PR. So, because of that, I agree that this should be treated as a bugfix and made the default. It doesn't really make sense to me to have a default behavior which has a good potential to break the code it is run on. Honestly I don't even think that should be an option. Just my $0.02. Thanks for the robust discussion, everyone!

[blutorange]

@ayusharma What's your current opinion on this? Should this still be an option? Should be enabled or disabled by default?

[ayusharma]

Hi @blutorange, @byara and I would like to keep it disabled by default.

[jasikpark]

Hi @blutorange, @byara and I would like to keep it disabled by default.

Is there a config value to enable using side-effect imports?

Automatically rewriting side-effect imports is similar to sorting imperative commands in alphabetical order. It's nice but it can break things

[IanVS]

I think this PR is an important bugfix, and have published https://www.npmjs.com/package/@ianvs/prettier-plugin-sort-imports which is a fork of this project, but includes this PR without reverting it. Maybe those of you with side-effect imports will find it useful.

[ngocdaothanh]

Question about @trivago/prettier-plugin-sort-imports v3.2.0:
This PR has been merged. But it seems it's not included in v3.2.0?

[ngocdaothanh]

This PR has been merged. But it seems it's not included in v3.2.0?

After reading the messages above, I see that this PR has been reverted 😞:
#111 (comment)

[Morriz]

@ayusharma / @byara why are you shipping buggy code and wasting time of so many users, when community contributes bug fixes alleviating these problems?

[d0z4rt]

Time to change cya, gl hf

[missbruni]

This is a huge shame, is there any plans to have this at least configurable in prettier as mentioned above? It's a deal breaker for us.

[artola]

This is a huge shame, is there any plans to have this at least configurable in prettier as mentioned above? It's a deal breaker for us.

@missbruni, my sincere thanks to the original authors for initiating this project. However, when I faced this issue, I found a solution by switching to @IanVS 's fork.
I highly recommend taking a look at Ian's package: https://www.npmjs.com/package/@ianvs/prettier-plugin-sort-imports.

[JustinGrote]

Ditched for @IanVS as well, Mantine React requires CSS imports in a specific order without having to dump to PostCSS, and Vite supports it just fine, so to have this tool mess it up wasted an hour of my time figuring out what went wrong and going to a better plugin.