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

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

Closed
jaydenseric opened this issue Oct 17, 2021 · 8 comments · Fixed by #69
Closed

Comments

@jaydenseric
Copy link

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:

@simonhaenisch
Copy link
Owner

simonhaenisch commented Oct 18, 2021

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.

@Eoksni
Copy link

Eoksni commented Nov 14, 2021

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.

@localpcguy

This comment was marked as resolved.

@simonhaenisch

This comment was marked as resolved.

@localpcguy

This comment was marked as resolved.

@simonhaenisch
Copy link
Owner

In case anyone wants to PR this:

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

  • 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 added a commit that referenced this issue Aug 15, 2022
Adds a new plugin option `organizeImportsSkipDestructiveCodeActions` that if set, runs `organizeImports` with the `skipDestructiveCodeActions` option enabled. This prevents unused imports from being removed.

Closes #37.
@simonhaenisch
Copy link
Owner

simonhaenisch commented Aug 15, 2022

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 added a commit that referenced this issue Aug 17, 2022
Adds a new plugin option `organizeImportsSkipDestructiveCodeActions` that if set, runs `organizeImports` with the `skipDestructiveCodeActions` option enabled. This prevents unused imports from being removed.

Closes #37.
@simonhaenisch
Copy link
Owner

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

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

Successfully merging a pull request may close this issue.

4 participants