Fix PowerShell MSIX (Store) detection #3202
Conversation
ghost
added
| @@ -268,7 +268,7 @@ export class PowerShellExeFinder { | |||
| // Find the base directory for MSIX application exe shortcuts | |||
| const msixAppDir = path.join(process.env.LOCALAPPDATA, "Microsoft", "WindowsApps"); | |||
|
|
|||
| if (!fs.existsSync(msixAppDir)) { | |||
There was a problem hiding this comment.
Good call to remove this anyway:
| @@ -469,6 +475,17 @@ export function getWindowsSystemPowerShellPath(systemFolderName: string) { | |||
| "powershell.exe"); | |||
| } | |||
|
|
|||
| function fileExistsSync(filePath: string): boolean { | |||
There was a problem hiding this comment.
The node docs aren't wrong about this kinda being an anti-pattern.
fs.exists() is an anachronism and exists only for historical reasons. There should almost never be a reason to use it in your own code.
In particular, checking if a file exists before opening it is an anti-pattern that leaves you vulnerable to race conditions: another process may remove the file between the calls to fs.exists() and fs.open(). Just open the file and handle the error when it's not there.
fs.exists() will be deprecated.
We're essentially making our own fs.existsSync and I question if we need it. As the code is written right now, this does seem to be the best option, but I'd want to review why we're checking if a file exists, then checking if a folder exists, etc.
| try { | ||
| // This will throw if the path does not exist, | ||
| // and otherwise returns a value that we don't care about | ||
| fs.lstatSync(filePath); |
There was a problem hiding this comment.
This probably ought to just be fs.statSync since we don't care about the information, but we may care that the target file exists. I don't really care though, as the code as written is (presumably) tested, and it's not really our problem if the symlink exists but its target does not.
|
Weird, @rjmholt the CI tests passed on this PR but then failed on the merge to master. |
PR Summary
Fixes #3181.
The MSIX exe is a symlink and node's file test API returns false for those.
This fixes that so we now detect the PowerShell MSIX installation properly again.
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets.Please mark anything not applicable to this PR
NA.WIP:to the beginning of the title and remove the prefix when the PR is ready