-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Dispatch event for all removed entries #34773
Dispatch event for all removed entries #34773
Conversation
Sounds like something we should have, I'd just be cautious about possible performance impact on large folder deletion. Did you attempt to fix a specific case with this? |
Performance wise, I think that the change simply move the iteration work from the listener to the dispatcher. Meaning, when a folder is deleted, instead of having to look for children in every listener, we do it once on the dispatcher side. So we are comparing the load of emitting more events to the load of listing directories multiple times. I think it also makes more sense regarding the naming of the event. If you listen to I two specific cases:
|
Ping for review :) |
I'd like to know what @icewind1991 thinks I vaguely remember that some events are purposefully only triggered on the container and not on all children, for performance reasons |
🏓 @icewind1991 for some input |
/rebase |
Signed-off-by: Louis Chemineau <louis@chmn.me>
91e6c73
to
2830eea
Compare
Not the best in case of performance but probably better than the alternatives that can be done easily |
This is kinda a breaking change. Should be added here: #37039 |
When removing a folder, dispatch the event for all the deleted entries and not only for the folder.
Does it make sense, or was the old behavior on purpose ?