-
-
Notifications
You must be signed in to change notification settings - Fork 586
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
handle promise rejection when a symlink's target does not exist. Fixe… #1010
Conversation
Is there a reason why this was not merged? At least it fixes #955 while also not adding more test failures (some of the tests fail on the current main branch anyway, on my machine). |
reason is simple: it's hard to understand which use cases this will break. We have millions of users, and the tests aren't really comprehensive, since they fail on CIs. I know that this is not a reason to halt progress, but we need more reviews of each PR etc. |
Uhh, actually, in this case, I don't see how it can break anything, since if the exception is ever triggered, it currently just kills the entire Node.js process? |
thanks @paulmillr ! Yeah, I totally appreciate that as the usage of a library approaches large numbers, the chance that any changes will break somebody approaches 1 😅 |
…s #955