-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Fix completion of partial identifier inside a JsxExpression #52572
Conversation
@microsoft-github-policy-service agree |
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. |
That being said, if this is all due to a deprecated flag, do we need to make the client stop sending that flag? |
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 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.
While the new TypeScript/src/server/session.ts Lines 2248 to 2252 in 2a8436c
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. |
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 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! :)
@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 |
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