Core | Component: Resolve config.events callbacks at dispatch time - #874
Open
rokotyan wants to merge 2 commits into
Open
Core | Component: Resolve config.events callbacks at dispatch time#874rokotyan wants to merge 2 commits into
config.events callbacks at dispatch time#874rokotyan wants to merge 2 commits into
Conversation
rokotyan
force-pushed
the
claude/stale-event-handlers-218abc
branch
2 times, most recently
from
August 14, 2026 21:08
be45c7a to
27597a7
Compare
Re-renders every 100ms, which keeps the 500ms event re-bind window occupied, and passes a new `click` closure through `config.events` on every render. Each closure reports which render built it, so a non-zero lag means a callback from an earlier configuration ran. A read-modify-write selection toggle shows what that costs: clicking an already selected point used to re-select it instead of clearing the selection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rokotyan
force-pushed
the
claude/stale-event-handlers-218abc
branch
from
August 14, 2026 21:34
27597a7 to
1ceed71
Compare
`_bindEvents` captured the events map when it attached the listener, and `setConfig` replaces `this.config` with a newly merged object. The bound closure therefore kept calling the callbacks of an outdated configuration until the next re-bind, which `render()` throttles by 500ms. A consumer that re-renders in bursts and passes closures capturing state got handlers that read stale state: a read-modify-write handler compared against a selection that had already changed and re-selected an element instead of clearing it. Pass the events map as a getter and look the callback up when the event fires, so callback identity stops mattering. The component's own default events and the user-defined `.user` ones go through the same path. This also fixes handler removal. Dropping an entry from `config.events` used to leave the old listener firing indefinitely, because a re-bind only attaches callbacks that are still in the map and nothing detached the old one; it now resolves to nothing and the listener is a no-op. The throttle stays for the DOM churn it was added for: Graph, LeafletMap and TopoJSONMap re-bind on every frame of a pan, zoom or layout interaction, where re-running `selectAll` and `.on()` over every rendered element is expensive. It is now orthogonal to callback correctness. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rokotyan
force-pushed
the
claude/stale-event-handlers-218abc
branch
from
August 14, 2026 21:41
1ceed71 to
27b500f
Compare
rokotyan
marked this pull request as ready for review
August 14, 2026 21:44
Contributor
Author
|
@lee00678 This is also ready for review |
This file contains hidden or 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
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.
Problem
Callbacks passed through
config.eventskeep running from an outdated configuration aftersetConfig. A consumer that re-renders in bursts — a dashboard-wide cross-filter, say — and passes closures capturing state gets handlers that read stale state. A read-modify-write handler then makes the wrong decision: clicking an already-selected element compares against a selection that has already changed and re-selects it instead of clearing it.Root cause
_bindEventscaptured the events map at the moment it attached the listener, and resolved the callback out of that captured object:setConfigdoesthis.config = merge(this._defaultConfig, config), andmergedeep-clones — sothis.config.eventsis a new object on every update. The attached closure still points at the previous one.Re-binding is what refreshes it, and that happens at the end of
render()through_setUpComponentEventsThrottled(500ms). So during a burst of updates the previously bound callbacks stay live until the trailing invocation. The throttle only bounds how long the callback is stale — the capture is what makes it stale at all. Where re-binding is delayed further (containers schedule rendering insiderequestAnimationFrame, which a background tab suspends), the callback never refreshes.Fix
_bindEventsnow takes the events map as a getter and resolves the callback when the event fires rather than when the listener is attached. Callback identity stops mattering, so no re-bind is needed on a config change. Both call sites go through it, so the component's own default events and the user-defined.userones behave identically:No public API change; the
eventsconfig shape is untouched.The throttle stays
Graph,LeafletMapandTopoJSONMapcall_setUpComponentEventsThrottled()directly on every frame of a pan / zoom / layout interaction. Unthrottled, each frame would re-runselectAllplus.on()across every rendered element. It still earns its keep for selector and DOM churn — it is now simply orthogonal to callback correctness. I added a comment at the declaration recording why it exists, since that wasn't written down anywhere.Also fixed: handler removal
Removing an entry from
config.eventspreviously left the old listener firing indefinitely — a re-bind only attaches callbacks that are still in the map, and nothing detached the old one. It now resolves to nothing and the listener is a no-op.Known residual
A newly added
(selector, eventType)pair still has no listener attached until the next throttled re-bind (≤500ms). This is the same pre-existing exposure that brand-new DOM nodes have always had. Closing it needs an events-shape check that bypasses the throttle, which lands inconsistently:Tooltip,BulletLegend,FlowLegendandRollingPinLegenddon't callsuper.setConfig— they inlinethis.config = merge(...)themselves — so any logic added there would silently skip those four. Happy to follow up if you'd like it closed.Reproducing
Added a dev example: Component Events → Component Events: Stale Handlers.
It re-renders every 100ms to keep the 500ms re-bind window occupied, and each click closure reports which render built it. Any non-zero lag means an outdated callback ran. It also drives the read-modify-write selection toggle described above.
Before this change, clicking a point reported a lag of 5 renders — exactly the 500ms window at a 100ms cadence — and the selection stayed stuck instead of toggling off. After, the lag is 0 and the toggle behaves.
Testing
pnpm run buildpasses for all six packages (ts, angular, react, svelte, vue, solid), andpnpm run build:dev.@types/node, so the build alone does not catch a dangling type reference).5→0; selection stuck at2→ toggles2⇄null.this.eventspath still fires — dispatchingmouseenter/mouseleaveon a Scatter point still sets_forceShowLabeland raises the node.cypress/e2e/tooltip.cy.tshas 7 failures on the built dev app, but they are identical with and without this change — pre-existing and unrelated (CypressensureElIsNotCoveredactionability errors). Flagging in case it's news.