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(fuzzy-finder) fs.lstatSync throws Exception if not a file or dir #944

Conversation

schadomi7
Copy link
Contributor

Identify the Bug

While trying to fix a self-hosted teletype version, I noticed the fuzzy-finder is broken with teletype and it is because of this:

If you try to use the fuzzy-finder while connected via teletype, it fails with an exception.
This is caused by a check if the uri is a directory. The used fs.lstatSync will throw if given path is not found.
Previously it used a fs-plus method, which did not throw.
Because it throws, the else branch is never reached for the teletype URIs.

Description of the Change

wrap fs.lstatSync in a try-catch block

Alternate Designs

I would love to do something like this

    } else if (fs.lstatSync(uri, {throwIfNoEntry: false})?.isDirectory()) {

unfortunately throwIfNoEntry is only available in newer versions of fs.

Alternative 2: Could revert to fs-plus, the old code did not throw
Alternative 3: It could be sufficient to explicitly check if it is a teletype URI, I do not know if there could be other types of URI.
Teletype URIs all start with atom://teletype/

Possible Drawbacks

Code could be harder to read.

Verification Process

I used the fuzzy-finder, did not crash. Also did not crash with teletype.

Release Notes

N/A

If it throws the Exception, other valid uris such as atom://teletype/...
will not be evaluated in the else branch
The old fs-plus did not throw.
Newer versions of fs.lstatSync allow for option parameter to control if it throws.
Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Sorry this took some time to check up on.

But thanks a ton for contributing! This code honestly looks fantastic for me, and thanks for clarifying the use of throwIfNoEntry since that'd be my knee jerk reaction here.

Otherwise this looks good to me, and seems like it should be good to merge with all tests passing.

Anyone else have any input? Or think we are good to land this in time for v1.115.0?

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

If I recall correctly, fs-plus is using try/catch under the hood in these situations anyway.

So, this instance of just manually re-adding the try/catch seems fine to me as well.

Thanks from me also for the contribution!

@confused-Techie
Copy link
Member

@confused-Techie confused-Techie merged commit 3411fb7 into pulsar-edit:master Mar 16, 2024
104 checks passed
@DeeDeeG DeeDeeG mentioned this pull request Mar 22, 2024
12 tasks
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.

3 participants