Skip to content
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: replaced call to fs.pathExists() with fs.existsSync() #1572

Conversation

jeffb-sfdc
Copy link

Replaced call to fs.pathExists() with fs.existsSync().

Reasons:

  1. pathExists() has been deprecated
  2. My team writes an extension for VS Code. I sometimes need to turn on caught exceptions and uncaught exceptions, but this is impossible to do with the cspell extension installed, since findSettingsFiles() looks for a dozen different config files and thrown an exception for every file it can't find (and it does this every 10 seconds)

@@ -70,7 +70,7 @@ function findSettingsFiles(docUri?: Uri, isUpdatable?: boolean): Promise<Uri[]>
.map((root) => configFileLocations.map((rel) => path.join(root, rel)))
.reduce((a, b) => a.concat(b), []);

const found = possibleLocations.map(async (filename) => ({ filename, exists: await fs.pathExists(filename) }));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for picking up on this.

Please use fileExists from 'common-utils/file.js';

Copy link
Author

Choose a reason for hiding this comment

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

I looked into it, but fileExists() throws exceptions, which is the point if this PR. Any reason why not to use fs.pathExists()?

Copy link
Collaborator

Choose a reason for hiding this comment

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

fs.existsSync will block, slowing the search down.

Under normal circumstances, fileExists should not throw an exception. It catches the exception thrown by fs.access and returns false.
https://github.com/streetsidesoftware/vscode-spell-checker/blob/main/packages/__utils/src/file.ts#L5

@Jason3S
Copy link
Collaborator

Jason3S commented Dec 6, 2021

@jeffb-sfdc,

Forgive me, I had not fully read your reasons.

Reasons:

  1. pathExists() has been deprecated

This file uses fs-extra where under the hood it uses fs.access to determine if the file exists. fs.acces is not deprecated, nor is fs-extra.pathExists.

  1. My team writes an extension for VS Code. I sometimes need to turn on caught exceptions and uncaught exceptions, but this is impossible to do with the cspell extension installed, since findSettingsFiles() looks for a dozen different config files and thrown an exception for every file it can't find (and it does this every 10 seconds)

Yes, this can be very annoying. Let's see if another solution works for you. Changing the code from async to sync isn't a good option. This code exists because the VS Code file watcher had been very buggy and slow. I suspect that they many have fixed it by now. I'll take a look.

@Jason3S
Copy link
Collaborator

Jason3S commented Dec 6, 2021

@jeffb-sfdc
Copy link
Author

@Jason3S It looks like v2.0.14 fixes the issue, and I'm going to close this PR.

@jeffb-sfdc jeffb-sfdc closed this Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants