POF: Do not reorder imports if not necessary

[academo]

Based on our discussion here #90

TL;DR: Call sort imports only if more than 1 import from the same module was added.

Background

The current import all import_all and import current import_current algorithms will perform an organize import action after the edits are applied.

This behaviour exists to prevent situations where TS code actions require to import two dependencies from the same module. But this is not always the case for all situations.

Re-organizing imports on import can become problematic for code bases that don't have a standard for imports order, or that have one that doesn't match the default TS one.

Re-organizing imports on import all or current also creates more changes on the file that required, e.g. when a single line of code with a function call is added and after using import current or import all, the file gets much more changes related only to the imports order.

What the plugin does to auto-import all is loop through ts code-actions and apply them.

The change:

What my change does is keep track of how many imports of the same module had been applied, and if more than 1 was applied it would then perform an import sort[1]

[1] I checked the TS server and I can't find a way to only de-duplicate the imports, the sort and import are always together. It would be idea to have a TS action only to de-dupe the imports.

Extended background

There are widely two cases I considered for this PR:

1. TS suggests to import two modules from the same file that will result on incorrect duplicate imports

// file test.ts
export function test1();
export function test2();

// file imports.ts
import example from 'example';

example();
test1(); // test1 is not defined
test2(); // test2 is not defined

In this case, ts will suggest two actions:

• import test1 from './test';
• import test2 from './test';

Applying auto import (both code actions) will result in the following incorrect code:

// file imports.ts
import example from 'example';
import {test1} from './test';
import {test2} from './test'; //duplicated import

example();
test1(); 
test2(); 

2. TS suggests to import (or add) an import

// file imports.ts
import example from 'example';
import {test1} from './test';

example();
test1();
test2();  // test2 is not defined

Applying auto import (one code action). will result in the following correct code

// file imports.ts
import example from 'example';
import {test1, test2} from './test';

example();
test1(); 
test2();

Videos

Add to existing import (no sorting):

Import module (no sorting):

import two from the same module (sorting):

Add two to existing import (no sorting)

[jose-elias-alvarez]

I like this idea! It's definitely cleaner than adding a separate option. I'll test it out soon.

One question that popped into my head as I looked at the code: Can we use a simpler heuristic to determine if the edit is adding to an existing import statement? For example:

1. If the edit contains multiple lines, we can assume it's adding a new import.
2. If an edit contains just one line and contains a comma, we can assume it's adding to an existing import (I don't think tsserver will ever break imports from a single module into multiple lines, but I could be wrong).
3. If an edit contains just one line without a comma, it's a new import.

I'm not sure whether those assumptions hold, but let me know what you think.

[academo]

@jose-elias-alvarez I did múltiple tests to find the best way to do it (see the videos I attached) and TS server does suggest múltiple imports for the same module. Regarding the comma I don't like it too much because technically could be a valid file name character in some file systems.

[academo]

@jose-elias-alvarez any follow up on this one?

[jose-elias-alvarez]

This works well. My worry is that the difference in behavior may seem arbitrary to users, since the way :TSLspImportAll works is pretty opaque. Perhaps we could add an option to always organize imports and set it to true by default to keep the current behavior?

[jose-elias-alvarez]

Suggested change

	local import = module:gsub("\n", "")
	local import = vim.trim(module)

Maybe it also makes sense to rename this to something like source, since import makes me think of the variable getting imported.

[academo]

This works well. My worry is that the difference in behavior may seem arbitrary to users, since the way :TSLspImportAll works is pretty opaque. Perhaps we could add an option to always organize imports and set it to true by default to keep the current behavior?

Yes this makes sense to me. I'll add an option for it. does avoid_reorder_import sound good?

[jose-elias-alvarez]

This works well. My worry is that the difference in behavior may seem arbitrary to users, since the way :TSLspImportAll works is pretty opaque. Perhaps we could add an option to always organize imports and set it to true by default to keep the current behavior?

Yes this makes sense to me. I'll add an option for it. does avoid_reorder_import sound good?

Maybe always_organize_imports?

[academo]

@jose-elias-alvarez I added the changes and the new option always_organize_imports (default to true). I also added documentation for it

[jose-elias-alvarez]

Other than my suggestion I think this looks good! I'm happy to merge this once commits are squashed.

[jose-elias-alvarez]

Suggested change

	local import = module:gsub("\n", "")
	local source = vim.trim(module)

I think I suggested this before, but I think changing the variable name to source and using vim.trim() makes it a little easier to parse this at a glance. (Of course, the other uses of the variable have to be updated, too.)

[academo]

@jose-elias-alvarez I made the variable change.

You can do a squash when you are merging the PR like this:

[jose-elias-alvarez]

Thanks!