NotificationsYou must be signed in to change notification settings
Fork13.2k
Star107k

Add string literal completions for`package.jsonimports` field#57718

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

andrewbranch merged 27 commits intomicrosoft:mainfromAndarist:pkg-json-imports-completions

Oct 31, 2024

Merged

Add string literal completions for`package.jsonimports` field#57718

andrewbranch merged 27 commits intomicrosoft:mainfromAndarist:pkg-json-imports-completions

Oct 31, 2024

Conversation

Copy link

Contributor

Andarist commentedMar 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 forpackage.jsonimports field

238651d

typescript-bot added the For Backlog BugPRs that fix a backlog bug label

Mar 11, 2024

Andarist commented

Mar 11, 2024

View reviewed changes

src/compiler/utilities.ts

		}

		/*@internal /
		exportfunctiongetPossibleOriginalInputPathWithoutChangingExt(

Copy link

ContributorAuthor

AndaristMar 11, 2024

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 ofgetOutputPathWithoutChangingExt:

TypeScript/src/compiler/emitter.ts

Lines 542 to 554 in3fca8c8

	functiongetOutputPathWithoutChangingExt(
	inputFileName:string,
	ignoreCase:boolean,
	outputDir:string\|undefined,
	getCommonSourceDirectory:()=>string,
	):string{
	returnoutputDir ?
	resolvePath(
	outputDir,
	getRelativePathFromDirectory(getCommonSourceDirectory(),inputFileName,ignoreCase),
	) :
	inputFileName;
	}

Andarist commented

Mar 11, 2024

View reviewed changes

src/compiler/utilities.tsShow resolvedHide resolved

Andarist commented

Mar 11, 2024

View reviewed changes

src/services/stringCompletions.ts OutdatedShow resolvedHide resolved

Andarist commented

Mar 11, 2024

View reviewed changes

src/services/stringCompletions.ts OutdatedShow resolvedHide resolved

Andarist commented

Mar 11, 2024

View reviewed changes

src/services/stringCompletions.ts OutdatedShow resolvedHide resolved

Andarist commented

Mar 11, 2024

View reviewed changes

src/services/stringCompletions.tsShow resolvedHide resolved

Andarist commented

Mar 11, 2024

View reviewed changes

src/services/stringCompletions.ts OutdatedShow resolvedHide resolved

Andarist commented

Mar 11, 2024

View reviewed changes

src/services/stringCompletions.tsShow resolvedHide resolved

Andarist commented

Mar 11, 2024

View reviewed changes

src/services/stringCompletions.tsShow resolvedHide resolved

Andarist commented

Mar 11, 2024

View reviewed changes

src/services/stringCompletions.tsShow resolvedHide resolved

Andarist commented

Mar 11, 2024

View reviewed changes

src/services/stringCompletions.ts OutdatedShow resolvedHide resolved

Andarist commented

Mar 11, 2024

View reviewed changes

tests/cases/fourslash/server/importCompletions_importsMap3.tsShow resolvedHide resolved

Copy link

ContributorAuthor

Andarist commentedMar 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

Mar 11, 2024

View reviewed changes

tests/cases/fourslash/server/importCompletions_importsMap5.tsShow resolvedHide resolved

format

2adb14c

Andarist mentioned this pull request

Mar 14, 2024

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

Closed

Copy link

Member

DanielRosenwasser commentedAug 23, 2024

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

Copy link

ContributorAuthor

Andarist commentedAug 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

Aug 23, 2024

andrewbranch added this to theTypeScript 5.7.0 milestone

Aug 23, 2024

DanielRosenwasser mentioned this pull request

Sep 9, 2024

TypeScript 5.7 Iteration Plan#59905

Closed

andrewbranch reviewed

Sep 13, 2024

View reviewed changes

Copy link

Member

andrewbranch left a 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!

src/compiler/utilities.tsShow resolvedHide resolved

tests/cases/fourslash/importCompletionsPackageJsonImportsPattern.tsShow resolvedHide resolved

Copy link

ContributorAuthor

Andarist commentedSep 25, 2024

@andrewbranch thanks for the review! I’ll try to clean this up asap

Merge remote-tracking branch 'origin/main' into pkg-json-imports-comp…

22b6148

…letions# Conflicts:#src/compiler/utilities.ts#src/services/stringCompletions.ts

Andarist force-pushed thepkg-json-imports-completions branch from06559e9 to22b6148Compare

September 28, 2024 21:21

Andarist added3 commits

September 28, 2024 23:47

fixed server fixtures

08eaa16

drop trailing slash support

083c5b8

fix support for trailing slash

b48f752

Andarist added14 commits

October 12, 2024 17:45

passmoduleSpecifierResolutionHost everywhere

db0d416

getMatchesWithPrefix helper

294c7da

add more tests but break some of the existing ones

0d0b881

fixed the incorrectly copy-pasted test

475c0b1

handleinputDirectories

a0ea1b9

add more tests, some of them still failing

c00448b

add test withallowImportingTsExtensions

e96bd5a

add extra tests with wildcard in the middle

c36d20a

add pkg scope test

0f4a6f8

remove broken path

6759383

limit imports lookup

48b2c78

dedupe imports/exports lookup

56a1563

handle declarationDir separately

b84468a

useValidationFlags.AllowWildcard

a80cdfa

Andarist commented

Oct 17, 2024

View reviewed changes

src/harness/vfsUtil.ts

		constvalue=normalizeFileSetEntry(files[key]);
		constpath=dirname ?vpath.resolve(dirname,key) :key;
		vpath.validate(path,vpath.ValidationFlags.Absolute);
		vpath.validate(path,vpath.ValidationFlags.Absolute\|vpath.ValidationFlags.AllowWildcard);

Copy link

ContributorAuthor

AndaristOct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I have no idea if this is completely correct. I added it to makepathCompletionsPackageJsonImportsSrcNoDistWildcard6 pass as it's using? in the component path component.

I have added that test based on the existingpathCompletionsPackageJsonExportsWildcard6. The difference is that the test I have added is afourslash/server test and it follows a slightly different codepath.

The existing one validates here:

TypeScript/src/harness/vfsUtil.ts

Lines 1104 to 1108 inb8e4ed8

	private_resolve(path:string){
	returnthis._cwd
	?vpath.resolve(this._cwd,vpath.validate(path,vpath.ValidationFlags.RelativeOrAbsolute\|vpath.ValidationFlags.AllowWildcard))
	:vpath.validate(path,vpath.ValidationFlags.Absolute\|vpath.ValidationFlags.AllowWildcard);
	}

And theValidationFlags.AllowWildcard was added therehere

On the other hand, thefourslash/server validates here, where I'm adding this comment. This codepath was added as part of#20763 - which proceeds the PR that introducedValidationFlags.AllowWildcard. So perhaps this was just a harmless omission in that newer PR. I don't know why this is a flag in the first place though so 🤷‍♂️

Copy link

Member

andrewbranchOct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@rbuckton can you advise?

Andarist added5 commits

October 17, 2024 11:04

allowJs accepts a boolean, not String(boolea), doh

96209a9

splitimportsLookup out ofnodeModulesDirectoryOrImportsLookup fo…

17b2aae

…r granular reuse

Relax check forinputExtension

22e4d49

swapmatchingSuffixes to change the priority order for `trimPrefixA…

a18104e

…ndSuffix`

fmt

36bed48

Copy link

ContributorAuthor

Andarist commentedOct 17, 2024

@andrewbranch it's ready for re-review :)

I could also use a build of this to test it more easily in a real project, and not only in the test harness ;p cc@jakebailey

Andarist requested a review fromandrewbranch

October 17, 2024 10:51

Copy link

Member

jakebailey commentedOct 31, 2024

@typescript-bot pack this

Copy link

Collaborator

typescript-bot commentedOct 31, 2024•
edited
Loading

Starting jobs; this comment will be updated as builds start and complete.

Command	Status	Results
`pack this`	✅ Started	✅ Results

Copy link

Collaborator

typescript-bot commentedOct 31, 2024•
edited
Loading

Hey@jakebailey, I've packed this intoan installable tgz. You can install it for testing by referencing it in yourpackage.json like so:

{    "devDependencies": {        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/164040/artifacts?artifactName=tgz&fileId=7D1CD9129128DE0FC3646A76F014BB023412AEA343119330949BF42DF191661C02&fileName=/typescript-5.7.0-insiders.20241031.tgz"    }}

and then runningnpm install.

There is also a playgroundfor this build and annpm module you can use via"typescript": "npm:@typescript-deploys/pr-build@5.7.0-pr-57718-9".;