Skip to content

Fix completion of partial identifier inside a JsxExpression #52572

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

Merged

Conversation

pyBlob
Copy link
Contributor

@pyBlob pyBlob commented Feb 2, 2023

This originated from a typo (logical or instead of and) introduced here: #51855

I tried to use fourslash tests, but this didn't work, because they can only pass "includeInsertTextCompletions" to the preferences. This bug can only be triggered when passing the deprecated "includeInsertTextCompletions" to the CompletionsRequest.

Fixes #52288

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Feb 2, 2023
@pyBlob
Copy link
Contributor Author

pyBlob commented Feb 2, 2023

@microsoft-github-policy-service agree

@jakebailey
Copy link
Member

Hm. I didn't realize that you were saying you couldn't write a fourslash test because fourslash didn't have an option (I assumed it was some sort of hard to reproduce race). It might be easier just to add the option to fourslash.

@jakebailey
Copy link
Member

That being said, if this is all due to a deprecated flag, do we need to make the client stop sending that flag?

@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Feb 2, 2023
Copy link
Member

@gabritto gabritto left a comment

Choose a reason for hiding this comment

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

This looks like the right fix, but ideally we'd like to have a fourslash test instead of a unit test. Could you explain why passing includeInsertTextCompletions to the preferences in a fourslash test didn't work? It shouldn't make a difference whether includeInsertTextCompletions is passed to preferences or in the completions request itself, so maybe there is a bug in the fourslash test runner and we can look into that.

@pyBlob
Copy link
Contributor Author

pyBlob commented Feb 3, 2023

While the new includeInsertTextCompletions in the preferences should always override the deprecated includeInsertTextCompletions, this is not the case. I had a short look over the two remaining references of the deprecated property, and I think I found why it is like that. Will try this tomorrow. Moving the deprecated property before the spread of the preferences should solve this:

...convertUserPreferences(this.getPreferences(file)),
triggerCharacter: args.triggerCharacter,
triggerKind: args.triggerKind as CompletionTriggerKind | undefined,
includeExternalModuleExports: args.includeExternalModuleExports,
includeInsertTextCompletions: args.includeInsertTextCompletions,

That being said, if this is all due to a deprecated flag, do we need to make the client stop sending that flag?

Yes, when the mentioned change works out, you should be able to make the client stop sending that flag without a change in behaviour, and finally get rid of the deprecated property.

Copy link
Member

@gabritto gabritto left a comment

Choose a reason for hiding this comment

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

I replaced the unit test with a fourslash test (and I checked that the fourslash test repros the issue), hope you don't mind. Thank you for the fix! :)

@pyBlob
Copy link
Contributor Author

pyBlob commented Feb 4, 2023

@gabritto Thanks for the fourslash test. I just came to the same conclusion while I've seen your commit pop up in the browser.

What confused me was my assumption that the deprecated property and the new preference have the same name, which is not the case. Apart from that, the old property and the new preference are merged correctly, and the client in vscode should be able to seamlessly switch to using the new preference.

@gabritto
Copy link
Member

gabritto commented Feb 4, 2023

@gabritto Thanks for the fourslash test. I just came to the same conclusion while I've seen your commit pop up in the browser.

What confused me was my assumption that the deprecated property and the new preference have the same name, which is not the case. Apart from that, the old property and the new preference are merged correctly, and the client in vscode should be able to seamlessly switch to using the new preference.

I had the same moment of confusion with includeCompletionsWithInsertText and includeInsertTextCompletions, not realizing there were two different properties 😅

@gabritto gabritto merged commit 00eb220 into microsoft:main Feb 4, 2023
@pyBlob pyBlob deleted the fix-jsx-expression-complete-identifier branch February 4, 2023 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pressing dot breaks jsx
4 participants