A number of improvements to the VSCode extension #543
Conversation
…sharpier running which prevents upgrading it
belav
force-pushed
the
better-extension-vscode
branch
from
2dd8798
to
132dd40
Compare
| @@ -32,27 +32,3 @@ jobs: | |||
| dotnet-version: '6.0.x' | |||
| - run: | | |||
| dotnet build Src/CSharpier.MsBuild/CSharpier.MsBuild.csproj | |||
| # run_vscode_tests: | |||
There was a problem hiding this comment.
Uh... is this intentional? Running tests on PR seems useful.
There was a problem hiding this comment.
They were failing randomly 50% of the time and it doesn't seem worth the effort to try to fix them. It is pretty fast to test them manually when the extension changes, which shouldn't be very often.
| private spawnProcess = (csharpierPath: string) => { | ||
| const csharpierProcess = spawn("dotnet", [csharpierPath, "--pipe-multiple-files"], { | ||
| private spawnProcess = (csharpierPath: string, workingDirectory: string) => { | ||
| let csharpierProcess = spawn("dotnet", [csharpierPath, "--pipe-multiple-files"], { |
There was a problem hiding this comment.
Uh... is the reason you're converting this to let the rant you had on discord?
let vs const is kind of useful for preventing accidental reassignment. It's probably worth following the entire rest of the industry on this one, rather than assuming we know better 😏
There was a problem hiding this comment.
I have accepted that I have lost the battle to convince others that prefer-let is superior and changed these back to const. I hold out hope that eventually others will come over to the dark side with me.
| formatFile(content: string, fileName: string): Promise<string> { | ||
| this.process.stdin.write(fileName); | ||
| formatFile(content: string, filePath: string): Promise<string> { | ||
| this.process.stdin.write(filePath); | ||
| this.process.stdin.write("\u0003"); |
There was a problem hiding this comment.
So, I just realized that \u0003 is our general purpose delimiter between fields of our ad-hoc serialization format.
If one of these gets dropped (probably because of a code bug on our part, but platform issues also do happen), it'll get stuck in a state where it's getting the content when expecting the name and vice-versa.
You should consider ending a file/job/request/message with a double \u0003 (to essentially distinguish the message delimiter from the delimiter of fields within a message) so that it will eventually self-recover from the bad state.
There was a problem hiding this comment.
I think changing that now would be kind of a headache. To make sure old versions of extensions work with the new double delimited version of csharpier, the double delimiter would need to be opt in with a flag, and then all the plugins would need another version check to determine if they should expect the double delimiter or not. I am leaning towards wait and see if it is a problem, and if so then add the double delimiter.
|
|
||
| let timeoutHandle: NodeJS.Timeout; | ||
| let setupKillRunningProcesses = () => { | ||
| // TODO we can't detect when the terminal gets focused |
There was a problem hiding this comment.
I'm not sure if I like this as the workaround very much. It means that every 15 seconds my vs-code will take a lot longer to save and will feel "sluggish". It'll also be non-deterministic (unless you already know this is the rule), which IMHO makes it feel even slower.
Doing "Reload Window" (or maybe a new action we create called "Restart CSharpier", kind of like the "Restart Omnisharp" one in the official plugin) after updating csharpier doesn't seem like a bad experience. Especially considering csharpier updates happen once every few weeks at most, whereas editor focus/timeout happens all the time.
We can even print a message reminding users to do so when installing/upgrading csharpier via a post-install script:
https://stackoverflow.com/questions/39034123/can-i-add-a-custom-step-to-a-nuget-install
There was a problem hiding this comment.
The problem is that without us killing off csharpier, the user can't update csharpier at all. The command hangs and then eventually errors with the following.
Failed to uninstall tool package 'csharpier': Access to the path
'C:\Users\bela\.dotnet\tools\.store\csharpier\0.13.0' is denied.
There is no clear indication it is vscode that is keeping csharpier running. I figured out it was the extension because I know how the extension works, but the average user of csharpier would not have that knowledge.
My goal was to make the slow formatting invisible to the user, but I did find one edge case where a user will have a slow format document. #558
There was a problem hiding this comment.
I had a new thought laying in bed this morning!
Make sure all the extensions use csharpier from a custom tool path, so that any user commands to install/upgrade/uninstall csharpier don't run into locked files and the extension doesn't need to continually kill and restart csharpier.
The extension would need to detect when the user changes versions. Or have an action to reload csharpier like you suggested. https://docs.microsoft.com/en-us/dotnet/core/tools/global-tools has details about the custom tool paths abilities.
There was a problem hiding this comment.
Right, I keep forgetting that windows doesn't allow updating executables in the background.
It's terrible that everyone has to keep doing workarounds like this because windows won't fix their shit, but oh well... 🤷
This handles the fixes for #531, #533 and #493