TS: Add setting to prompt users about workspace tsdk#95566
TS: Add setting to prompt users about workspace tsdk#95566mjbvz merged 2 commits intomicrosoft:masterfrom
Conversation
|
|
DuncanWalter
force-pushed
the
ts-prompt-use-workspace-tsdk
branch
from
33084c7 to
5b0f687
Compare
There was a problem hiding this comment.
Thank you for taking a look at this!
I'm having trouble following some of the logic and how all the events interact with each other. Here's a quick breakdown of the expected behavior for a few different cases:
User opens a workspace without a tsdk setting
No change.
User opens a workspace with a tsdk setting and enablePromptUseWorkspaceTsdk != true
No change.
User opens a workspace for the first time that has tsdk set and enablePromptUseWorkspaceTsdk = true
- We start the normal server.
- At this point, we also show a prompt to the user asking if they would like to switch to the workspace version.
- If the user says no, we remember that and don't prompt them again.
- If the user says yes, we perform the equivalent of manually selecting the workspace version using the
Select TypeScript versioncommand. This automatically restarts the TS Server with the workspace version
User opens a workspace and is using global TS version. Then tsdk or enablePromptUseWorkspaceTsdk is written into the workspace setting
Show the same prompt as above. (Make sure that using the Select TypeScript Version command does not trigger a prompt)
User opens a workspace that they previously configured to use the workspace version
The user should never be prompted to switch.
This seems to mostly match what the PR is doing but make sure to try out the edge case too.
For the code changes, I'd try to avoid touching the TypeScriptVersionProvider and keep the logic centralized to the version manager
extensions/typescript-language-features/src/commands/allowPromptUseWorkspaceTsdk.ts
Outdated
Show resolved
Hide resolved
DuncanWalter
force-pushed
the
ts-prompt-use-workspace-tsdk
branch
from
5b0f687 to
187dbaf
Compare
mjbvz
left a comment
mjbvz
left a comment
There was a problem hiding this comment.
Looks good. Just a small change requested and then I think we should be good to merge
| this._register(this._versionManager.onDidPickNewVersion(() => { | ||
| this.restartTsServer(); | ||
| })); | ||
| this._register(this._versionManager.onEnteredPromptWorkspaceTsdkState(() => { |
There was a problem hiding this comment.
Looks like this event is now only being used to call back into the version manager, so let's removed it and just have the version manager call promptUseWorkspaceTsdk
|
|
||
| private updateForPickedVersion(pickedVersion: TypeScriptVersion) { | ||
| public async promptUseWorkspaceTsdk(): Promise<void> { | ||
| const workspaceVersion = this.versionProvider.localVersion; |
There was a problem hiding this comment.
Can you please also make sure the user is not prompted if typescript is installed locally but typescript.tsdk is not set
There was a problem hiding this comment.
It follows the desired behavior in local testing. The check to see whether the prompt should runs is checking for the tsdk setting in configuration here. The intent is that promptUseWorkspaceTsdk should never be called in situations where the user shouldn't be prompted. Confusingly, promptUseWorkspaceTsdk reads the local version from the versionProvider to get a reference to the instance of TypeScriptVersion. configuation.localTsdk is a string, which isn't enough to set the new TS version AFAIK
|
Preemptively bumping this because I think I pushed most recently during the githooks disruption yesterday- not sure if it sent notifications out properly. No rush / not trying to nag |
|
Thanks! This will be in the next vs code insiders build and is scheduled to go out with VS Code 1.45 Does this PR address the original issue you opened as well? |
|
It addresses the underlying problem, so I'll close the issue 👍 |
3 participants
This PR addresses #95250, #65546
Adds a setting to the
typescript-language-featuresextension which enables prompting users to switch to the workspace tsdk specified in the workspace settings. The prompts can be permanently dismissed on a per-workspace basis. As a formality, also adds a hidden command that reenables the prompts (which I'd be happy to rip out- just seemed weird to have non-reversible internal state).I'm all ears for any and all naming/wording improvements. One potential change I'm on the fence about is displaying the relative workspace path to the tsdk in the prompt.
I opted to make the
VersionProviderclass a disposable with an event emitter as opposed to making the client's update callback more complicated. I also decided to keepextensionContextaccess limited to theVersionManager. I attempted a couple other implementations, but ultimately found it difficult to prevent theVersionProviderandVersionManagerfrom becoming entangled in a confusing way.The prompt is triggered when all of:
enablePromptUseWorkspaceTsdksettingAre there examples of any tests for similar features I should look at mimicking?
cc @mjbvz