[WIP] feat(import all): implement priority

[jose-elias-alvarez]

Closes #42.

This implementation prioritizes imports in the following (descending) order:

1. Imports from a source already used in the current file, identified by Add.
2. Imports from local sources (imperfect, a bit more on this below).
3. Imports from open buffers. It'll check buffer names to see if they match the source and also scan buffer content to see if a matching import statement exists.
4. Imports from non-local sources.

There's a small performance tradeoff from the priority check, especially with big imports, but I think it's acceptable given the advantages, so I've enabled the feature by default but added the option import_all_disable_priority to force the old (VS Code-compliant) behavior.

For 2 in the list above, there's no definite marker to distinguish between local and non-local imports. Relative paths that start with ./ are a good indicator, but tsconfig.json has a baseUrl option that messes with this. Ultimately, I think we can't predict this perfectly, but we can add more patterns and improve accuracy (I've started with src for now).

For 3, I added the option import_all_scan_buffers (number) to limit the number of buffers that will be scanned. On my end, scanning buffers is surprisingly fast, but I've defaulted the option to 0 to prevent issues with other workflows / bigger projects.

[jose-elias-alvarez]

I'm finding that scanning buffers is remarkably fast, even with a large (100+) number of buffers loaded and a relatively large (25+) number of missing imports. I'm going to use this at work this week with import_all_scan_buffers = 100 and see if I run into any issues, since I think it'll provide a better experience if we enable it by default.

[jose-elias-alvarez]

I've made a change to priority to prefer local sources over buffer sources, which seems reasonable and is better for performance, but I'm happy to hear feedback. It'd be simple to implement customizable priorities if that's something users are interested in, but I prefer to have sensible defaults.

[non25]

Ultimately, I think we can't predict this perfectly, but we can add more patterns and improve accuracy (I've started with src for now).

Project I'm working on uses src as baseUrl, does that mean it will understand that common/icons/EditIcon or lib/somefunc are local?

Or maybe it is better to alias it somehow, like ~/lib/somefunc? That way it could decide everything based on the import path, avoiding the need to read directory tree? :thinking:

I'm going to test this on the following week too. Will post my findings.

Thanks a lot. That's really helpful. :slightly_smiling_face:

[jose-elias-alvarez]

Project I'm working on uses src as baseUrl, does that mean it will understand that common/icons/EditIcon or lib/somefunc are local?

It won't, since just based on the path, both of those could be local or non-local.

I want to think a bit more about potential solutions to the baseUrl issue. One idea is to get the current file's "base" directory relative to the root directory (so root/src/common/icons/EditIcon would resolve to /src), run a shallow scan to get directories within that directory (in your example, /common and /lib), and then check path names against those directories.

Another, maybe better, idea is to run a command like git ls-files to get the actual list of project files, then compare the source against that list. That's probably as close to perfect accuracy as we can get, and other plugin functionality already depends on Git, so it's not a big change.

I want to make sure that the added complexity and overhead justifies the benefit, though, and even if we can't handle this situation, the other improvements make a big difference, so I'm not unhappy to leave it as-is (even though half of the projects I work on also use baseUrl).

Let me know how it goes with the PR. I'll be force pushing changes to it throughout the week if I find any issues, and if everything looks good I'll merge it at the end of the week.

[jose-elias-alvarez]

Update: I think the git files route is the right solution, and the overhead is negligible on the small-to-medium projects I work on, so I just pushed an implementation. @non25 Let me know how it works for you, especially in terms of performance.

[non25]

I've used TSLspImportAll for a few times now and it is much faster than importing one by one. If I import one by one, I will wait for the next import code action, because tsserver has to do its thing and understand the changes it did. :confused:

Strange thing though, two times it preferred react material table to common/Table, but common/Table was imported in the other buffer, and material table wasn't.

Maybe it has something to do with other imports from material, unrelated to Table. Feels like it looks for where to import from, not what to import. Will post after more thorough investigation.

The only thing I miss now is interactive window with possible import choices.

Thanks. :slightly_smiling_face:

[jose-elias-alvarez]

Glad to know it's working well for you overall. The current implementation will prioritize imports from a source already used in the current file, which I think is what is causing the behavior you describe. We could change the behavior to instead prefer importing from sources also used in other buffers, in which case I think a logical order would be:

1. Import from sources that are actually loaded in Neovim
2. Import from sources used in another open buffer
3. Import from sources already used in the same file
4. Import from local sources
5. Import from other sources

Does that make more sense to you? I'm starting to think that allowing configuration might be the way to go, but I'd still prefer to get the defaults right.

And yep, implementing the interactive window should be straightforward. I'm thinking of adding an option to let users choose whether they want to choose between conflicting imports or instead let the plugin attempt to resolve it for them. I'll hopefully have time to work on that next week but will probably merge this before then.

[non25]

I would prefer local sources everytime. For example, when I copy component, delete all imports, move it to the other place and use import all

import { makeStyles } from "@material-ui/core";
import { Table, TableBody, TableCell, TableHead, TableRow } from 'common/Table';
// ... other stuff

it makes it like this:

import { Table, makeStyles } from "@material-ui/core";
import { TableBody, TableCell, TableHead, TableRow } from 'common/Table';
// ... other stuff

Other than that, everything is fine. Thanks. :slightly_smiling_face:

[vuki656]

Does that make more sense to you? I'm starting to think that allowing configuration might be the way to go, but I'd still prefer to get the defaults right.

I feel like defaults and an option to configure it yourself would be awesome. You'll never satisfy everyone :smile:

[jose-elias-alvarez]

Thank you guys for the feedback! I've restructured the way priority is calculated in a way that should (hopefully) provide better accuracy out-of-the-box while also enabling configuration. I've added a new option, import_all_priorities, with the following structure:

import_all_priorities = {
    buffers = 4, -- loaded buffer names
    buffer_content = 3, -- loaded buffer content
    local_files = 2, -- git files or files with relative path markers
    same_file = 1, -- add to existing import statement
}

The numbers define the weight given to each factor, meaning that you could, for example, always prioritize local files by setting local_files = 999 or de-prioritize same file imports by setting same_file = 0. (Setting import_all_priorities = nil disables the feature entirely).

Other changes: I found that buffer scanning really doesn't affect performance, so I've enabled it by default. I've also implemented @non25's suggestion and added the option import_all_select_source, which overrides priority and instead asks the user to interactively choose from available sources.

If everything looks good, I'll again test it at work this week and merge it shortly.

[non25]

I've used the new version for two days now and came to a conclusion that automatic imports won't do what I expect from it in the current project. Too many exports with the same name, but different source location. But, now we have import_all_select_source, which solves everything. :slightly_smiling_face: Thanks a lot! It is immensely useful.

[jose-elias-alvarez]

Great! I imagine that's the right solution for larger projects. I'm happy with this for now, so I'm going to merge it and leave additional feedback for further issues.