prettier-plugin-sort-imports icon indicating copy to clipboard operation
prettier-plugin-sort-imports copied to clipboard

Do not reorder side effect imports

Open blutorange opened this issue 2 years ago • 18 comments

Apologies if this has been mentioned already, I did not find any issue for, though.

Is your feature request related to a problem?

A side effect import is an import that only imports a module for its side effects, without importing any values (i.e. where ImportDeclaration.specifiers is an empty array). These are currently sorted just like any other imports. Since those imports are explicitly used for side-effects, changing the order will usually result in a different behavior.

Now side effects are something that should be avoided if possible, and library code should have side effects. However, there are some valid use cases, such as loading mocks for tests, mock-fs or loading global libraries in an index file of a wep app, etc. For example:

import "./setup-test-environment"; // setup some required globals on which some module rely unfortunately
import { something } from "./a-module-to-test";

The above would be re-ordered, which breaks the test.

Describe the solution you'd like

Side effect imports should not be reordered. eslint-plugin-import does not reorder them either, for the same reason.

Since it is hard to know which side effect imports affect which other imports, I would suggest the following solution: treat a side effect import as a boundary that splits the imports into two groups, the imports before the the side effect import and the imports after the side effect import. Then only sort the imports before and after the side effect import as you would normally. This is I think the easiest to implement and also the safest. If users want to have side effect at the top, they can just move the side effect imports to the top, and the formatter will leave them there.

I made this a feature, but I would almost consider it a bug. By default, side effect imports should not be reordered. An option could be added for sorting these imports too, but I'm not sure if that's really necessary. It also goes against prettier's philosophy of limiting the options as much as possible.

So for example:

// Side effect import 1
import "S1";

// Start group 1
import {} from "b";
import * as A from "c";
// End group 1

// Side effect import 2
import "S2";

// Start group 2
import foo from "d";
import { bar } from "e";
// End group 2

// Side effect import 3
import "S3";

In the example above, keep the side effect imports as they are. Sort the imports in group 1 and group 2 using whatever options are set. Do not move any import from group 1 to group 2 or vice-versa.

Describe alternatives you've considered

Another alternative I can think of is to simply keep the relative ordering of the side effect imports, but allow moving non side effect imports anywhere. Or always just put side effect imports at the top. Both of this possibilities have the disadvantage they the may be unsafe: putting an import from before a side effect import after it may break it because it does not expect that side effect; putting an import from after a side-effect before it may break it because it relies on the side effect. It's also unclear how imports with and without side effects should be ordered relatively to each other.

blutorange avatar Nov 14 '21 17:11 blutorange

Probably also solves #95, if I understand that issue correctly.

blutorange avatar Nov 14 '21 17:11 blutorange

@blutorange Same for me. We would like to use this plugin, but the ordering must be "perfect", as the users do not expect their code to break unexpectedly just because of saving a file for example.

artola avatar Dec 05 '21 14:12 artola

Same for us, I just crashed all of our unit tests because for our jest setup we need a specific import order: https://github.com/nrwl/nx/issues/6361#issuecomment-994922109.

Also, I didn't find any workaround, except adding those files to the .prettierignore.

NiklasPor avatar Feb 02 '22 08:02 NiklasPor

@ayusharma This bug is really a showstopper, sometimes for a few files you can use @NiklasPor workaround mentioned above, but it does not scale when you run a huge codebase with many devs that should be aware, making the code really fragile. If a test breaks, probably you will find it early, but if the change is more subtle, like CSS order, then it will go direct on your face.

artola avatar Feb 02 '22 08:02 artola

These issues make me wonder if anybody who's using this plugin actually has any side effect imports in files that are not in the .prettierignore. If nobody does, then #111 wouldn't even affect existing users.

blutorange avatar Feb 02 '22 08:02 blutorange

@blutorange #111 is great solution. Thanks! Now I hope to get Trivago's team support to get this bug solved.

artola avatar Feb 02 '22 08:02 artola

Actually, our workaround looks now a bit different. I just overwrote the sorting order for those specific files. Still not a great solution:

// prettierrc.js
overrides: [
    {
      files: 'test-setup.ts',
      options: {
        importOrder: ['jest.*', '<THIRD_PARTY_MODULES>'],
      },
    },
  ]

NiklasPor avatar Feb 02 '22 08:02 NiklasPor

Has this issue being solved?

DavidArmendariz avatar Feb 08 '22 03:02 DavidArmendariz

The current state of the issue, as far as I know, is that it's not a bug and also working as intended. There was the suggestion to add an option (disabled by default) that enables #111. If anybody wants to take that PR and add said option, feel free to, but personally I'm not interested in a solution that is "broken" by default. Perhaps somebody else has an idea how to solve this in a way that can be enabled by default?

blutorange avatar Feb 08 '22 17:02 blutorange

@blutorange is it time to fork?

artola avatar Feb 08 '22 22:02 artola

@blutorange I agree that you PR would not bother anyone, and I don't understand the maintainers point, but maybe we can proceed step by step. The first step would be to activate the new behavior you implemented with an option and leave the default behavior as is. Once the new behavior will have proven its worth, the next step would be to set it as the default, and an option would activate the former behavior. If every case is handled with the new behavior, and if the option become useless, the last step would be to remove this option.

cvolant avatar Feb 21 '22 09:02 cvolant

Hmm, yeah, forking this project just for changing one default option is not worth the effort. If anything I was thinking about a different sorting plugin with a different philosophy (closer to prettier, opinionated, not many settings, more emphasis on correctness). But for now that's just a thought.

So yes, since this plugin already quite a few options and keeping the number of options low does not seem to be a major goal (?), one more options could be the way to go. I might add that option to the PR when I have time for it, or sometime else does it in the mean time.

Edit: If we make this an option, we can go a little step further and call it unsortableImports, which lets people define which imports should not be sorted, with the default being ["unnamed"]. This would solve issues with e.g. named CSS exports that should not be sorted.

blutorange avatar Feb 21 '22 09:02 blutorange

I think that #111 is an important bugfix, and that this plugin is not suitable for production use without it. So, I've published a fork which is currently the same as this package, but with https://github.com/trivago/prettier-plugin-sort-imports/pull/111 included (without any kind of option necessary). It can be found at https://www.npmjs.com/package/@ianvs/prettier-plugin-sort-imports for anyone else who might find it useful.

IanVS avatar Feb 26 '22 02:02 IanVS

Just had some annoying CSS issues because the import of the library's CSS was moved to after our custom style overrides. Love this library, but it doesn't make sense for us to ignore prettier completely for a file just because of the imports. Will move to the fork while this issue is not resolved.

ianldgs avatar Jun 01 '22 09:06 ianldgs

We moved too for the exact same reason.

cvolant avatar Jun 01 '22 16:06 cvolant

Any forward motion towards unifying these efforts? Can #111 not be merged or reworked? Or @IanVS's fork be merged into this one? It sux to have a whole fork for what is in essence a bug, as this plugin is unreliable without the fix.

@blutorange it seems you are most involved with this, so may I ask what is the blocker here?

Morriz avatar Jun 10 '22 17:06 Morriz

@Morriz The blocker is that the maintainers of this project consider this bug fix as "it fails the basic idea of the plugin"

blutorange avatar Jul 01 '22 23:07 blutorange

@Morriz FWIW, my fork now has a few features that this project does not have, and gets a fair number of npm downloads. I use it in a few of my own projects, and continue to maintain it. In case that eases your mind about using a fork.

IanVS avatar Jul 25 '22 15:07 IanVS