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.

Sign up for GitHub

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

Jump to bottom

Add string literal completions for `package.json` `imports` field #57718

Draft

Andarist wants to merge 2 commits into microsoft:main from Andarist:pkg-json-imports-completions

Contributor

Andarist commented Mar 11, 2024 •

edited

Loading

closes #52460
closes #57680
closes #57777

Currently, this only has tests based on #55015 but I still have to add more


          Add string literal completions for package.json imports field

238651d

typescript-bot added the For Backlog Bug label

Andarist commented

View reviewed changes

src/compiler/utilities.ts

                       [Extension.Tsx, Extension.Ts, Extension.Jsx, Extension.Js];
               }
+              /** @internal */
+              export function getPossibleOriginalInputPathWithoutChangingExt(

Contributor Author

Andarist Mar 11, 2024

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; 
        
           }

Andarist commented

View reviewed changes

src/compiler/utilities.ts

		@@ -6288,6 +6290,21 @@ export function getPossibleOriginalInputExtensionForExtension(path: string) {
		[Extension.Tsx, Extension.Ts, Extension.Jsx, Extension.Js];

Contributor Author

Andarist Mar 11, 2024

tangent: would it make any sense to limit this to avoid returning js/jsx for path with ts/tsx?

Member

andrewbranch Sep 13, 2024

It seems like it. If it doesn’t affect anything negatively, I'd say go for it.

Andarist commented

View reviewed changes

src/services/stringCompletions.ts Outdated

		host: LanguageServiceHost,
		moduleSpecifierResolutionHost: ModuleSpecifierResolutionHost \| undefined

Contributor Author

Andarist Mar 11, 2024

TODO: it was annoying to pass moduleSpecifierResolutionHost here from all of the callers but I probably should do that

Andarist commented

View reviewed changes

src/services/stringCompletions.ts

+                      matches = concatenate(matches, inputMatches);
+                  }
+                  matches = concatenate(matches, directories);

Contributor Author

Andarist Mar 11, 2024

TODO: this lacks inputDirectories

Andarist commented

View reviewed changes

src/services/stringCompletions.ts


		function trimPrefixAndSuffix(path: string): string \| undefined {
		if (inputBaseDirectory && inputBaseDirectory !== baseDirectory) {

Contributor Author

Andarist Mar 11, 2024

TODO: a helper has to be introduced to deduplicate this branch with the ones that creates original matches

Andarist commented

View reviewed changes

src/services/stringCompletions.ts

Comment on lines 849 to +850

		isExports: boolean,
		isImports: boolean,

Contributor Author

Andarist Mar 11, 2024

it's somewhat annoying to have those separate parameters but I need to distinguish between those 2 cases. Alternatively, I could introduce an enum

Andarist commented

View reviewed changes

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)

Contributor Author

Andarist Mar 11, 2024

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

Andarist commented

View reviewed changes

src/services/stringCompletions.ts

+                              }
+                              if (!seenPackageScope) {
+                                  const packageFile = combinePaths(ancestor, "package.json");
+                                  if (seenPackageScope = tryFileExists(host, packageFile)) {

Contributor Author

Andarist Mar 11, 2024

TODO: this has to be heavily deduplicated with the existing branch that handles exports

Andarist commented

View reviewed changes

src/services/stringCompletions.ts

@@ @@ -938,10 +951,41 @@ function getCompletionEntriesForNonRelativeModules( @@
                           }
                       }
                       if (!foundGlobal) {
+                          let seenPackageScope = false;

Contributor Author

Andarist Mar 11, 2024

TODO: add a test case that will show that it's needed

Andarist commented

View reviewed changes

src/services/stringCompletions.ts

		): readonly NameAndKind[] {
		if (!endsWith(path, "*")) {

Contributor Author

Andarist Mar 11, 2024 •

edited

Loading

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.

Andarist commented

View reviewed changes

src/services/stringCompletions.ts

@@ @@ -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;

Contributor Author

Andarist Mar 11, 2024

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.

Andarist commented

View reviewed changes

tests/cases/fourslash/server/importCompletions_importsMap3.ts

+              });
+              verify.completions({
+                  marker: ["2"],
+                  exact: ["foo.js"],

Contributor Author

Andarist Mar 11, 2024

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.

Contributor Author

Andarist commented Mar 11, 2024

@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.

Andarist commented

View reviewed changes

tests/cases/fourslash/server/importCompletions_importsMap5.ts Outdated

Contributor Author

Andarist Mar 11, 2024

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.


          format

2adb14c

Andarist mentioned this pull request

Intellisense is not working for self-references and # imports of typescript files in vscode #57777

Closed

Member

DanielRosenwasser commented Aug 23, 2024

@Andarist did you still want to pursue this? We're looking to get this done in the TS 5.7 timeframe.

Contributor Author

Andarist commented Aug 23, 2024

@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

andrewbranch self-assigned this

andrewbranch added this to the TypeScript 5.7.0 milestone

DanielRosenwasser mentioned this pull request

TypeScript 5.7 Iteration Plan #59905

Open

andrewbranch reviewed

View reviewed changes

Member

andrewbranch left a comment

Yeah, this looks on the right track to me, thanks!

src/compiler/utilities.ts

		@@ -6288,6 +6290,21 @@ export function getPossibleOriginalInputExtensionForExtension(path: string) {
		[Extension.Tsx, Extension.Ts, Extension.Jsx, Extension.Js];

Member

andrewbranch Sep 13, 2024

It seems like it. If it doesn’t affect anything negatively, I'd say go for it.

tests/cases/fourslash/importCompletionsPackageJsonImportsPattern.ts

+              verify.completions({
+                  marker: ["1"],
+                  exact: ["#something.js"],

Member

andrewbranch Sep 13, 2024

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

For Backlog Bug