Fix data received by delayed on-signal-patch#1123
Fix data received by delayed on-signal-patch#1123jpeg729 wants to merge 2 commits intostarfederation:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug where the on-signal-patch plugin with modified timing (throttle/debounce/delay) would sometimes not fire when combined with filters. The issue occurred when a second patch signal event replaced a delayed event before execution, and the new event didn't match the filters.
Changes:
- Refactored the event handler to apply filtering before timing modifications, ensuring that only events matching the filters are passed to throttled/debounced/delayed callbacks
- Removed the
runningflag that was previously used to prevent concurrent execution, as it's no longer needed with the new structure
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const watched = filtered(filters, (evt as CustomEvent<JSONPatch>).detail) | ||
| if (!isEmpty(watched)) | ||
| { | ||
| timedCallback({...evt, detail: watched}) |
There was a problem hiding this comment.
Using the spread operator on a CustomEvent object may not properly preserve all event properties, as properties like type, bubbles, cancelable, target, and others are getters on the Event prototype chain, not own properties. This means {...evt, detail: watched} will only copy enumerable own properties and won't include these inherited event properties. Consider creating a new CustomEvent instance with the appropriate properties, or verify that the timing functions and rx callback don't need access to standard event properties beyond detail.
| const watched = filtered(filters, (evt as CustomEvent<JSONPatch>).detail) | |
| if (!isEmpty(watched)) | |
| { | |
| timedCallback({...evt, detail: watched}) | |
| const customEvt = evt as CustomEvent<JSONPatch> | |
| const watched = filtered(filters, customEvt.detail) | |
| if (!isEmpty(watched)) { | |
| const patchedEvent = new CustomEvent<JSONPatch>(customEvt.type, { | |
| detail: watched, | |
| bubbles: customEvt.bubbles, | |
| cancelable: customEvt.cancelable, | |
| composed: customEvt.composed, | |
| }) | |
| timedCallback(patchedEvent) |
|
I removed the Maybe you prefer a proper type check on the type of the event, but I think I am right in saying that the old code didn't so a type assertion is not worse. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Here is a more minimalist example https://codepen.io/jpeg729/pen/OPXmZML |
I confirm that I have read the contribution guidelines and will include a clear explanation of the problem, solution, and alternatives for any proposed change in behavior.
Problem Statement
The on-patch-signals plugin with modified timing will sometimes not fire when combined with filters. When a second patch signals event occurs before the value is executed, the event is replaced, which is good unless the new event doesn't match the filters.
Proposed Solution
Filter events before applying throttling.
Alternatives Considered
We could leave it as is and propose workarounds based on data-on-interval or similar, but the fix seems straightforward and the upside seems clear.
Fixes #1122