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

Use the `source.sortImports` code action instead to avoid deleting "unused" imports

Open jaydenseric opened this issue 2 years ago • 6 comments

The source.sortImports code action is similar to the what organize imports does, except it doesn't doesn't remove "unused" imports.

Automatic removal of "unused" imports by a Prettier plugin is unacceptable, for a few reasons:

  1. Automatically deleting imports that could have intended side-effects is more risky than just reordering them.

  2. It destructively modifies the code you are working on. Often you begin writing a new module by writing the imports you expect to use, then start writing the module code. If you trigger a Prettier format at any point (e.g. by saving the file) before you have used all the imports in your module code, the imports unhelpfully get stripped out.

  3. It destroys JS code examples in markdown readme files that demo how to import things. In these situations the imports are expected to be unused, since the JS is just an example snippet. The result after running Prettier on the markdown is all the JS fenced code blocks containing just imports are empty. Here is an example of "unused" imports within markdown:

    Screen Shot 2021-10-17 at 2 26 17 pm

    I've found it difficult to reliably reproduce when the TS language server considers the imports "unused", when using Prettier with your prettier-plugin-organize-imports in a VS Code editor open to a markdown file with JS fenced code blocks containing single import statements, they don't get deleted on format. But, the exact same markdown when Prettier formatted programmatically as part of the jsdoc-md readme generation does result in deleted imports.

Somewhat related:

  • https://github.com/simonhaenisch/prettier-plugin-organize-imports/issues/24
  • https://github.com/simonhaenisch/prettier-plugin-organize-imports/issues/25

jaydenseric avatar Oct 17 '21 03:10 jaydenseric

Automatic removal of "unused" imports by a Prettier plugin is unacceptable

Sorry not sorry that I disagree 🙃 Not sure why you think it's a good way to start an issue like this... "unacceptable" is a pretty hostile word for something that currently 337 people have starred because apparently they find it very useful 🤷🏻‍♂️

  1. Automatically deleting imports that could have intended side-effects is more risky than just reordering them.

Please read the disclaimer in the readme. Using modules for global side effects is not a good practice IMO and should just be avoided. Side-effects-only imports (e. g. import 'foo') are not removed by source.organizeImports afaik. If you're relying on side effect imports a lot, then this plugin is the wrong choice for you. Also, reordering side-effect imports actually sounds more risky to me, because with side-effects the order might actually matter (e. g. modular firebase imports).

2. It destructively modifies the code you are working on. Often you begin writing a new module by writing the imports you expect to use, then start writing the module code. If you trigger a Prettier format at any point (e.g. by saving the file) before you have used all the imports in your module code, the imports unhelpfully get stripped out.

I totally don't write code like that and if you do, then yeah this plugin is probably not the best option for you. You might be better off using trivago's prettier-plugin-sort-imports which has a very different approach and is way more configurable.

I write code by just writing the code and letting VS Code auto-import the modules as I type, and I only save when I'm done with something I want to run (shout-out to VS Code Hot Exit). Because of that, this plugin is perfect for my way of working (and many others it seems). In my opinion it would suck if I had to manually remove unused imports (e. g. if I imported stuff temporarily to try something out). I barely look at the imports anymore and it's made my coding life a lot easier.

I've also done a lot of code reviews and people forgetting to remove unused imports is way too common. (Yeah linters can take care of that but also the linter's editor plugin shouting at you while you code is very annoying)

3. It destroys JS code examples in markdown readme files that demo how to import things.

This is the valid point I see in your issue.

For one, you can already work around this by adding a comment like this to your markdown

<!-- // organize-imports-ignore -->

so that the file is skipped by the plugin.

Another possible solution going forward is to add a boolean removeUnusedImports config option to the plugin that switches to using source.sortImports if false (true by default). I'm open to a PR for that, seems pretty straightforward.


But, the exact same markdown when Prettier formatted programmatically as part of the jsdoc-md readme generation does result in deleted imports.

Looking at that piece of code, it might be that the inferred parser is different when running Prettier in VS Code vs running Prettier via the CLI/programmatically.

simonhaenisch avatar Oct 18 '21 10:10 simonhaenisch

Another possible solution going forward is to add a boolean removeUnusedImports config option to the plugin that switches to using source.sortImports if false (true by default). I'm open to a PR for that, seems pretty straightforward.

I would totally love to see this implemented. I press ctrl-s all the time just for the sake of it (habit "from the old days") and sometimes I lose my imports unintentionally. I'll try to find some time at the weekend to raise a PR but ... no promises.

Eoksni avatar Nov 14 '21 20:11 Eoksni

Just a reminder that not everyone is going to use this with VSCode, so if options are added that explicitly use VSCode functionality (rather than Prettier or Typescript), that should be made very clear in the README or at least in that option. ~I know the README currently mentions 'This is the same as using the "Organize Imports" action in VS Code.' but that isn't quite accurate given this discussion, I believe.~ (edit: I got a bit confused about that last bit there)

localpcguy avatar May 25 '22 16:05 localpcguy

Hey @localpcguy not sure i follow... this plugin is for Prettier and has nothing to do with VS Code per-se. It uses a method of the TypeScript language service API called source.organizeImports, and VS Code (when in TypeScript language mode) has an action called "Organize Imports", that calls the same method of the same API. So yeah, using the VS Code action or running Prettier with this plugin does effectively the same thing to your import statements.

Maybe you misunderstood something, or else you can clarify why the readme would be inaccurate?

There's not gonna be any options specifically for VS Code, not even sure how that would be possible... this is not an editor plugin but a Prettier plugin.

simonhaenisch avatar May 25 '22 21:05 simonhaenisch

I think it was the link in the original comment in this issue - "The source.sortImports code action" that lead me to believe that if this was enacted as an option, it would use the VSCode source action (for sortImport). And I didn't see sortImport as an option on the TS language API. Very possible I was just misunderstanding and I could be mistaken and it is a TS thing - if so, sorry for any confusion, I haven't delved that deep into the TS language service API.

I mainly took the time to respond cause of this comment which led me to believe it could be implemented (again, it's fine if so).

Another possible solution going forward is to add a boolean removeUnusedImports config option to the plugin that switches to using source.sortImports if false

The Readme bit is my bad, I was mixing up this proposed functionality (sortImports) with the existing (organizeImports).

localpcguy avatar May 25 '22 23:05 localpcguy

In case anyone wants to PR this:

  • Expose a new boolean option sortOnly (see https://prettier.io/docs/en/plugins.html#options)

https://github.com/simonhaenisch/prettier-plugin-organize-imports/blob/5ba4618ba24e98e88a781d4fb9b7bfc00caa3cfd/lib/organize.js#L15

  • change ☝️ this to sth like
const method = options.sortOnly ? languageService.sortImports : languageService.organizeImports;

const fileChanges = method({ type: 'file', fileName }, {}, {})[0];

(assuming those two methods have the same signature)

  • add some tests

simonhaenisch avatar Jun 12 '22 19:06 simonhaenisch

Finally had some time to make a PR 👆 for this in case anyone is keen to have a look/add some comments before I merge it.

simonhaenisch avatar Aug 15 '22 19:08 simonhaenisch

Just released 3.1.0 so in case you want to use this, you can upgrade, then add organizeImportsSkipDestructiveCodeActions: true to your Prettier config.

simonhaenisch avatar Aug 17 '22 06:08 simonhaenisch