-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
chore: use kqueue backend of notify on macOS #21028
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a difference in performance?
Hard to tell until all of the PRs land. This one needs review: #18072 |
cli/tests/unit/fs_events_test.ts
Outdated
@@ -32,7 +32,7 @@ async function getTwoEvents( | |||
const events = []; | |||
for await (const event of iter) { | |||
events.push(event); | |||
if (events.length > 2) break; | |||
if (events.length >= 2) break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: leave a comment here describing why it might be > 2 in some cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM one nit
This reverts commit 646afdf.
Reverts #21028 Reason: https://github.com/notify-rs/notify/blob/main/notify/src/kqueue.rs#L79-L81 Need to wait for the watcher thread to spawn otherwise we hit flakes --------- Signed-off-by: Divy Srivastava <[email protected]>
Fixes #20996
"macos_fsevent" feature of notify links us to CoreFoundation on macOS.