fix(events): Simplify filter function, add new enqueueEvent function
#8539
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Remove the broken BlockChange event reordering code from filter. Instead, do necessary event reordering (correctly, this time) in a new enqueueEvent function used by fireInternal.
Since we don't filter in undo any more, and filtering in reverse order causes problems, prepare to remove this opportunity for error.
Simplify filter by having it consider only adjacent events in the queue for merging. Previously any events in the queue could potentially be merged if they were of suitable (typically identical) .type, even if other events had occurred between them (with the sole exception of BlockMove events, which would only be merged with adjacent events). This could potentially result in replay failures during undo/redo/mirroring, though it is unknown whether any such problems occurred in practice.
- Use for…of loop where appropriate. - Make filter reason-merging code more concise. - Use arrow functions when calling Array.prototype.filter.
gonfunko
approved these changes
Aug 22, 2024
1 task
This was referenced Aug 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The basics
The details
Resolves
Fixes #2037, #8225
Proposed Changes
BlockChangeevent reordering code fromfilter. Instead, do necessary event reordering in a new (and hopefully more robust and less buggy)enqueueEventfunction called fromfireInternalas events are fired.filterwithforward=false.filterby having it consider only adjacent events in the queue for merging. Previously any events in the queue could potentially be merged if they were of suitable (typically identical).type, even if other events had occurred between them (with the sole exception ofBlockMoveevents, which would only be merged with adjacent ones). This could potentially result in replay failures during undo/redo/mirroring, though it is unknown whether any such problems occurred in practice.for…ofloop where appropriate.Array.prototype.filter.Reason for Changes
filterthat has been the source of multiple current and previous bugs, and actively breaks other attempts to fix those bugs (e.g. by emitting events in the correct order to start with).filterso it is easier to reason about its behaviour.Test Coverage
enqueueEvent.filter, none of which fail due to the changes made in this PR.Additional Info
Strictly speaking, making the
forwardparameter offilterdefault totrueis a breaking change, as JS users would previously have observed it defaulting to falsyundefined. My reasoning to overlook this is that I suspect very few developers use this function directly (to the point that I think we should consider deprecating its export), and the likelihood of any of them calling it with no second parameter, intending that to request filtering in reverse order, seems very remote.Nevertheless the change adding a default could be removed if desired.