feat: delegate action events

dummdidumm commented 2 weeks ago

This is a rough proposal to patch dom instances when they are passed to actions so that addEventListener delegates events. This ensures that people adding listeners imperatively will still have the event order preserved.

The code is a bit duplicative right now and it's missing cleanup/tests/logic to not patch twice, wanted to validate the idea first / get some opinions on this before proceeding.

We could also think about adding the same logic to bind:this.

Before submitting the PR, please make sure you do the following

[ ] It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
[ ] Prefix your PR title with feat:, fix:, chore:, or docs:.
[ ] This message body should clearly illustrate what problems it solves.
[ ] Ideally, include a test that fails without this PR but passes with it.

Tests and linting

[ ] Run the tests with pnpm test and lint the project with pnpm lint

changeset-bot[bot] commented 2 weeks ago

⚠️ No Changeset found

Latest commit: b8c32f51861cd405bab4f3238fd8dfd0813cd250

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

trueadm commented 2 weeks ago

Can't we just re-use the logic in local event handlers where we run the delegated workflow? Surely it's the same thing as here?

dummdidumm commented 2 weeks ago

Right now it's always just a single delegated listener per event because we don't allow multiple listeners per event type. But with actions you now can have potentially many of them, which complicates the whole thing. We could always use an array to simplify, not sure what implications that has on perf/memory

Rich-Harris commented 3 days ago

This does all make me a bit nervous. Have we tried the suggestion in https://github.com/sveltejs/svelte/issues/11516#issuecomment-2101495048 of adding event listeners in an effect (or rather in a microtask — we don't need the overhead of actually creating an effect, we just need things to happen in the right order)? If it's beneficial, we could presumably even save that behaviour for when an element has both events and actions.

dummdidumm commented 2 days ago

We decided to not pursue this. Instead, if the need arises, we can add import { addEventListener } from 'svelte/events' which does the delegation.

sveltejs / svelte