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: missing require to fs.rmSync. #538

Merged
merged 2 commits into from Aug 3, 2023

Conversation

richardo2016
Copy link
Contributor

Hello, this PR aims to solve this issue:

#523

I was actually surprised by this issue. It is a very obvious and easily fixable error. Why didn't any official member handle it before? Was it simply because the path that used rmSync was hardly triggered? But due to another issue with https_proxy, this path can be easily triggered on Windows.

@richardo2016
Copy link
Contributor Author

@shiftkey @niik Hi! Could you please do a review for this PR?

Copy link
Contributor

@tidy-dev tidy-dev left a comment

Choose a reason for hiding this comment

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

💖 Thanks for the fix.

My assumption is you are correct in that this code path is not hit frequently.

@tidy-dev
Copy link
Contributor

tidy-dev commented Aug 2, 2023

Hmm.. I see the checks are not running. @richardo2016 would you mind merging the main branch in to get a fresh commit to trigger the ci (and just update since this pr got a little stale, sorry about that) 🙏 ?

@richardo2016
Copy link
Contributor Author

Hmm.. I see the checks are not running. @richardo2016 would you mind merging the main branch in to get a fresh commit to trigger the ci (and just update since this pr got a little stale, sorry about that) 🙏 ?

No problem, I'll try merge main branch to see if the checks running.

@tidy-dev tidy-dev enabled auto-merge August 3, 2023 16:59
@tidy-dev tidy-dev merged commit 0f5a4f1 into desktop:main Aug 3, 2023
10 checks passed
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.

None yet

2 participants