Support both baseUrl and relative paths when adding missing import #19724
Conversation
ghost
requested a review
from 
| if (!options.baseUrl) { | ||
| return undefined; | ||
| function getRelativePathLength(relativePath: string): number { | ||
| if (startsWith(relativePath, "../")) { |
There was a problem hiding this comment.
use directorySeparator here instead
| } | ||
|
|
||
| // Prefer a relative import over a baseUrl import if it doesn't traverse up to baseUrl. | ||
| const relativeFirst = getRelativePathNParents(relativePath) < getRelativePathLength(getRelativePath(sourceDirectory, baseUrl, getCanonicalFileName)); |
There was a problem hiding this comment.
I am not sure i understand this condition..
should not this just be getRelativePathNParents(getRelativePath(sourceDirectory, baseUrl, getCanonicalFileName)) ?
There was a problem hiding this comment.
Added a comment, tell me if this could be done in some simpler way.
| } | ||
|
|
||
| /* | ||
| Prefer a relative import over a baseUrl import if it doesn't traverse up to baseUrl. |
There was a problem hiding this comment.
assuming that both source file and module file are under the baseUrl, would not it be sufficient to sort based on the number of levels u need to go up (getRelativePathLength) between (source, baseUrl) and (source, module)?
|
@Andy-MS We currently try to apply all the returned code actions. After this change, we would need to change our logic to instead show a selector. We originally tried to avoid this UX because it breaks up the typing flow |
|
@Andy-MS we should make sure completion always has one code action.. |
|
We will only use one code action, from the shorter choice of baseUrl or relative path. |
ghost
mentioned this pull request
Fixes #15888 and #19920
@mjbvz
CompletionEntryDetailshas aCodeAction[], how are you handling this in the user interface if there is more than one? For example an export from a particular source could have multiple ways to be imported.