emscripten-core / emscripten-core/emscripten
`JSEvents.registerOrRemoveHandler`: should the add path dedupe like the remove path does?
- Dominant language
- C++
- Stars
- 27.6k
- Forks
- 3.6k
- Avg merge
- 1d 1h
- Merged PRs (30d)
- 105
Description
`JSEvents.registerOrRemoveHandler` in `src/lib/libhtml5.js` deduplicates by `(target, eventTypeString)` on the **remove** path but not on the **add** path:
```js
registerOrRemoveHandler(eventHandler) {
...
if (eventHandler.callbackfunc) {
eventHandler.target.addEventListener(...); // always adds
JSEvents.eventHandlers.push(eventHandler); // never dedupes
} else {
for (var i = 0; i < JSEvents.eventHandlers.length; ++i) {
if (JSEvents.eventHandlers[i].target == eventHandler.target
&& JSEvents.eventHandlers[i].eventTypeString == eventHandler.eventTypeString) {
JSEvents._removeHandler(i--); // dedupe-and-replace
}
}
}
...
}
```
So calling e.g. `emscripten_set_keydown_callback(target, ud1, ..., cbA)` followed by `emscripten_set_keydown_callback(target, ud2, ..., cbB)` results in *both* listeners staying attached to the DOM target. Every browser `keydown` then fires `cbA` and `cbB` in order. The user has to know to clear the slot first by passing `NULL` as the callback.
Is this asymmetry intentional?
The current behavior is surprising: a name like `set_*_callback` reads as a setter (replace), not as a "push another listener". And there's no clean way for a caller to register a single handler idempotently without an unrelated `(target, NULL, 0, NULL)` clear first.
---
I just hit this issue in a project using SDL3 as the backend. The PR I sent to SDL3 explains everything more in detail: https://github.com/libsdl-org/SDL/pull/15511
Possible options:
1. **Status quo**: keep the current behavior; maybe document it more prominently.
2. **Dedupe on the add path**: make `registerOrRemoveHandler` remove any existing `(target, eventTypeString)` entry before pushing the new one. Behavioral change for any code that relied on stacking, but I'd guess that code is rare/accidental.
3. **New explicit API**: keep `emscripten_set_*_callback` as add-only, introduce `emscripten_replace_*_callback` (or a flag) for the dedupe-then-add semantics.
Thoughts?
Contributor guide
Assessment
This issue has not been assessed yet.