-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Add string literal completions for package.json
imports
field
#57718
base: main
Are you sure you want to change the base?
Conversation
@@ -6288,6 +6290,21 @@ export function getPossibleOriginalInputExtensionForExtension(path: string) { | |||
[Extension.Tsx, Extension.Ts, Extension.Jsx, Extension.Js]; | |||
} | |||
|
|||
/** @internal */ | |||
export function getPossibleOriginalInputPathWithoutChangingExt( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is kinda a reverse of getOutputPathWithoutChangingExt
:
TypeScript/src/compiler/emitter.ts
Lines 542 to 554 in 3fca8c8
function getOutputPathWithoutChangingExt( | |
inputFileName: string, | |
ignoreCase: boolean, | |
outputDir: string | undefined, | |
getCommonSourceDirectory: () => string, | |
): string { | |
return outputDir ? | |
resolvePath( | |
outputDir, | |
getRelativePathFromDirectory(getCommonSourceDirectory(), inputFileName, ignoreCase), | |
) : | |
inputFileName; | |
} |
@@ -6288,6 +6290,21 @@ export function getPossibleOriginalInputExtensionForExtension(path: string) { | |||
[Extension.Tsx, Extension.Ts, Extension.Jsx, Extension.Js]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tangent: would it make any sense to limit this to avoid returning js/jsx for path
with ts/tsx?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like it. If it doesn’t affect anything negatively, I'd say go for it.
src/services/stringCompletions.ts
Outdated
host: LanguageServiceHost, | ||
moduleSpecifierResolutionHost: ModuleSpecifierResolutionHost | undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: it was annoying to pass moduleSpecifierResolutionHost
here from all of the callers but I probably should do that
matches = concatenate(matches, inputMatches); | ||
} | ||
|
||
matches = concatenate(matches, directories); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: this lacks inputDirectories
|
||
function trimPrefixAndSuffix(path: string): string | undefined { | ||
if (inputBaseDirectory && inputBaseDirectory !== baseDirectory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: a helper has to be introduced to deduplicate this branch with the ones that creates original matches
isExports: boolean, | ||
isImports: boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's somewhat annoying to have those separate parameters but I need to distinguish between those 2 cases. Alternatively, I could introduce an enum
src/services/stringCompletions.ts
Outdated
@@ -875,7 +887,7 @@ function addCompletionEntriesFromPathsOrExports( | |||
if (typeof pathPattern === "string" || matchedPath === undefined || comparePaths(key, matchedPath) !== Comparison.GreaterThan) { | |||
pathResults.push({ | |||
matchedPattern: isMatch, | |||
results: getCompletionsForPathMapping(keyWithoutLeadingDotSlash, patterns, fragment, baseDirectory, extensionOptions, isExports && isMatch, host) | |||
results: getCompletionsForPathMapping(keyWithoutLeadingDotSlash, patterns, fragment, baseDirectory, extensionOptions, (isExports || isImports) && endsWith(keyWithoutLeadingDotSlash, '*'), isImports, compilerOptions, host, moduleSpecifierResolutionHost) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isExportsWildcard
parameter here is a little bit surprising. It feels like it could be checked further down the callee chain - but I'm not sure yet
} | ||
if (!seenPackageScope) { | ||
const packageFile = combinePaths(ancestor, "package.json"); | ||
if (seenPackageScope = tryFileExists(host, packageFile)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: this has to be heavily deduplicated with the existing branch that handles exports
@@ -938,10 +951,41 @@ function getCompletionEntriesForNonRelativeModules( | |||
} | |||
} | |||
if (!foundGlobal) { | |||
let seenPackageScope = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: add a test case that will show that it's needed
): readonly NameAndKind[] { | ||
if (!endsWith(path, "*")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also fix completions when *
is somewhere in the middle of paths
. The algorithm is largely the same between paths/exports/imports after all (I need to add a test case for this).
Note that nothing here fixes completions with multiple wildcards in the value - I don't want to work on that.
@@ -1072,9 +1153,11 @@ function getModulesForPathsPattern( | |||
|
|||
const normalizedSuffix = normalizePath(parsed.suffix); | |||
const declarationExtension = normalizedSuffix && getDeclarationEmitExtensionForPath("_" + normalizedSuffix); | |||
const matchingSuffixes = declarationExtension ? [changeExtension(normalizedSuffix, declarationExtension), normalizedSuffix] : [normalizedSuffix]; | |||
const inputExtension = isImports && normalizedSuffix ? getPossibleOriginalInputExtensionForExtension("_" + normalizedSuffix) : undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the logic in this function assumes the same extensions between potential inputs and outputs and stuff. This can impact the used globs but I think it's largely fine right now - unless somebody would come up with a compelling test case that should work differently.
}); | ||
verify.completions({ | ||
marker: ["2"], | ||
exact: ["foo.js"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This currently fails. I took this test case from https://github.com/microsoft/TypeScript/pull/55015/files#diff-c8e8521e91d6946ce7869359a143f8cf7b65ee15703a4e04c7d73dcbaa5682a4 but I think that perhaps it should just be removed/tweaked in both places. A trailing slash is already not supported by node@>=17 and it was deprecated before that. There is little value in supporting this - in my opinion, it would be better to discourage this completely.
In addition to that, I noticed that the previous segment gets suggested in a situation like this today:
{
"name": "pkg_test",
"exports": {
"./test/": "./"
}
}
import { bazinga } from 'pkg_test/test//*1*/'
At marker 1
I get test
again, it's imho a clear bug and I can fix it here - after we agree on not supporting this old deprecated pattern at all.
@andrewbranch I opened this as a draft because there are some minor cleanups to be done here. I'd appreciate an early review here though - in case there is something fundamentally wrong with this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need more test cases like this but with a pattern. Atm, I didn't add any special logic for declarationDir
- this "exact" match works solely based on the fact that it's exact. It doesn't even need to check if the corresponding file exists on the disk or not so it doesn't perform any output->input mapping etc. This has to be fixed.
@Andarist did you still want to pursue this? We're looking to get this done in the TS 5.7 timeframe. |
@DanielRosenwasser yes, it would be great if @andrewbranch could give this a quick look to check if im not doing anything overly wrong so I dont spend too much time on cleaning up things that will turn out to be wrong in the end ;p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this looks on the right track to me, thanks!
@@ -6288,6 +6290,21 @@ export function getPossibleOriginalInputExtensionForExtension(path: string) { | |||
[Extension.Tsx, Extension.Ts, Extension.Jsx, Extension.Js]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like it. If it doesn’t affect anything negatively, I'd say go for it.
|
||
verify.completions({ | ||
marker: ["1"], | ||
exact: ["#something.js"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to have a version of this test with allowImportingTsExtensions
where this appears as "#something.ts"
. (Pretty sure it will work without additional changes)
@andrewbranch thanks for the review! I’ll try to clean this up asap |
…letions # Conflicts: # src/compiler/utilities.ts # src/services/stringCompletions.ts
06559e9
to
22b6148
Compare
closes #52460
closes #57680
closes #57777
Currently, this only has tests based on #55015 but I still have to add more