Skip to content
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

Jump to bottom

Don’t offer import statement completions at from position #44125

Merged
merged 2 commits into from May 18, 2021
Merged

Don’t offer import statement completions at from position #44125

merged 2 commits into from May 18, 2021

Conversation

andrewbranch
Copy link
Member

❗ Hoping to get this fix into for 4.3. The current state is pretty disruptive if your muscle memory expects a completion for from but you get an import completion instead.

Fixes this:

import statement completions showing auto imports starting with 'f' at 'import {} f'

By making it this:

completions at 'f' now shows 'from' and no auto imports

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 17, 2021
andrewbranch
andrewbranch commented May 17, 2021
View reviewed changes
src/services/completions.ts
@@ -183,6 +185,20 @@ namespace ts.Completions {
return { isGlobalCompletion: false, isMemberCompletion: false, isNewIdentifierLocation: false, entries };
}

function specificKeywordCompletionInfo(keywords: readonly SyntaxKind[]): CompletionInfo {
Copy link
Member Author

Choose a reason for hiding this comment

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

Making a way to return specific keywords is extra credit beyond just fixing the obvious regression; the behavior in 4.2 was not great:

image

andrewbranch
andrewbranch commented May 17, 2021
View reviewed changes
src/services/completions.ts Outdated
@@ -183,6 +185,20 @@ namespace ts.Completions {
return { isGlobalCompletion: false, isMemberCompletion: false, isNewIdentifierLocation: false, entries };
}

function specificKeywordCompletionInfo(keywords: readonly SyntaxKind[]): CompletionInfo {
return {
isGlobalCompletion: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried making this false but the snippets for for loops and stuff still showed up 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Should it be false then?

@mjbvz any idea why that might be happening?

Copy link
Member Author

Choose a reason for hiding this comment

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

Set to false now despite seeing no difference locally

DanielRosenwasser
DanielRosenwasser requested changes May 17, 2021
View reviewed changes
Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

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

Seems right, but I feel like you should specify isGlobalCompletions as false (or at least get clarity on behavior), and you could also just switch to a cheaper lookup for name.

Once that's done, we can cherry-pick into release-4.3.

src/services/completions.ts Outdated Show resolved Hide resolved
@andrewbranch
Copy link
Member Author

@typescript-bot cherry-pick this to release-4.3

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 17, 2021 •
edited

Heya @andrewbranch, I've started to run the task to cherry-pick this into release-4.3 on this PR at afa4d05. You can monitor the build here.

@typescript-bot typescript-bot mentioned this pull request May 17, 2021
Merged
@typescript-bot
Copy link
Collaborator

Hey @andrewbranch, I've opened #44136 for you.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request May 17, 2021
@andrewbranch
Cherry-pick PR microsoft#44125 into release-4.3
badc741 
Component commits:
34b80a5 Don’t offer import statement completions at `from` position

afa4d05 Set isGlobalCompletion to false, use indexOf lookup
@andrewbranch andrewbranch merged commit 271e069 into microsoft:master May 18, 2021
@andrewbranch andrewbranch deleted the bug/import-statement-completions-from branch May 18, 2021 00:48
andrewbranch added a commit that referenced this pull request May 18, 2021
@typescript-bot @andrewbranch
Cherry-pick PR #44125 into release-4.3 (#44136)
22c3c24 
Component commits:
34b80a5 Don’t offer import statement completions at `from` position

afa4d05 Set isGlobalCompletion to false, use indexOf lookup

Co-authored-by: Andrew Branch <andrew@wheream.io>
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.3.2 milestone May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielRosenwasser DanielRosenwasser DanielRosenwasser approved these changes

@sheetalkamat sheetalkamat sheetalkamat approved these changes

@sandersn sandersn Awaiting requested review from sandersn

Assignees

@andrewbranch andrewbranch

Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Milestone
TypeScript 4.3.2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@andrewbranch @typescript-bot @DanielRosenwasser @sheetalkamat